Since these are examples, I would suggest getting rid of extraneous commented out code, and also highlight important lines of code such as this one. For example, is it important that it gets called before importing pyplot? What is it for? Perhaps a short docstring at the top of this example would help explain its purpose/goal that it is demonstrating?

same here, should be a class docstring

docstring is wrong nm, I was confused

numpy docstring please.

actually, does numpy doc play nice with properties?

done

Don't need the **, numpydoc should take care of the markup on the way out to html

Can you clarify this a bit. This is setting a property of NavigationBase to trigger the tool on a key stroke. This docstring and function name make it sound like this is modifying the Tool object in some way.

What do you think of renaming to set_keymap?, we don't have keymaps for anything else than tools.

How about update_keymap? 'set' makes me think it is being replaced. As this is a Navigation level function I think that is the correct sematics (even though you are replacing the keys for the given tool).

It might also be worth adding a append kwarg to skip removing all of the existing keys for the given tool.

rename done, not sure about append

is this done and not pushed? I am not seeing the code change....

I am finishing all the current comments and push the commit, give me 5 😉

How strongly do you feel about keeping both the singular and plural versions of this? The similarity (leading to many typos) and the difference in signature/what they can do is going to be confusing. If this function stays I think it needs to be able to pass args/kwargs through to the constructors.

Is there any terrifying input parsing we could use to mush this into one function? I am not seeing a way quickly...

@tacaswell, me and @fariza had a discussion on args/kwargs etcetrra over in fariza#11 where I introduced them.

I would have thought the difference in signature would solve the typos problem, I mean if you make a typo, you will get a python error telling you you passed the wrong number of arguments to the function.

We can think of renaming the methods (suggestions any one?), but playing magic with arguments is 👎

great, sold on 👎 for args magic (we have it else where where I do not like it, but supposedly users like that sort of thing)

What is the use case for bulk adding tools? Writing this loop in user code does not strike me as super onerous.

My worry with the typo is getting an exception about args being wrong (which leads you to check what you are passing in) rather than an AttributeError (which leads you to check your spelling).

It is more for backend implementation. It was done automatically at __init__ but @OceanWolf talked me out of that

Ooh, brain wave, how about:

for tool in six.iteritems(tools):
    self.add_tool(*tool)

Or get rid of it altogether, I think it has changed a lot (becoming a lot simpler), especially since we split the adding of tools to the toolbar out and into ToolContainer.addTool as @fariza mentions.

@OceanWolf where do you suggest to add that loop?
Don't you like the helper functions in backend_tools?

👍 On putting this helper function as a module-level function. Breaks the OO abstraction a bit, but it keeps the base objects simpler.

@OceanWolf That would unpack as self.add_tool(tool_name, value_in_tools_dict) which does not get you the next level of unpacking you need to pass to add_tool.

Yes, I like that too, brain just sluggish and getting distracted by quite a few conversations. I don't mind, just so long as no core-logic changes.

done

probably should do a # noqa here to keep pep8 from complaining. Might also need a comment explaining why to keep future devs from "fixing" this.

Sorry, I don't see the pep8 error here.

I think this should be if not tool_cls ?

Wouldn't be simpler to just have self._get_cls_to_instantiate raise if it fails? If you want to do something else in that case then just try...execpt and if you want to die (which I am assuming is the default case) you don't have to do anything.

I suspect @WeatherGod also has a pyflake/pylint turned on.

also, I am reading and commenting top-down. Sorry if some of these seem dumb in light of code I have not read yet.

done @tacaswell change, but still not see the pep8 error

Different versions of pep8? I find myself curious to learn about this as Travis doesn't complain.

Should this be done first in this function? Do we want to check to make sure that they are the same type? Again, why warn rather than raise?

I used warn because i don't think that adding twice the same tool represents a major fault and your program will keep working as expected (you already have the tool)

shouldn't this be self._tools[name].keymap ? Might as well allow for per-instance key binding.

The one who makes the sorting (by keymap) of the keypress events is NavigationBase.
It is easier to handleNavigationBase._keys dictionnary than looping throught all the tools finding their keymaps when an event occurs.
What we can do, if you insist in per-instance key binding, is to modify set_tool_keymap to set the tool keymap attribute at the same time.

Huh? per-instance key binding happens, this just sets the default keymap.

@fariza I think you should rename ToolBase.keymap to ToolBase.default_keymap as I think that would make it a lot clearer!

I don't quite follow this yet..

@OceanWolf agreed and done

@tacaswell set_tool_keymap(name, keymap) does per-instance key binding, it sets the keymap of the instance identified by name to the specified keymap. Here it takes the default keymap (a class level property) and binds it to the instance. Does that make it clearer for you?

Again, ask the instance object, not the class?

The radio_group is a class attribute, I don't think there is any difference if we access it from the class or from the object, is there?

class foo(object):
    bar = 5
    def __init__(self):
        self.baz = 7


A = foo()
B = foo()
A.bar = 'good morning!'

print()
print("A.bar: ", A.bar)
print("B.bar: ", B.bar)
print("foo.bar: ", foo.bar)

gives

A.bar:  good morning!
B.bar:  5
foo.bar:  5

so by looking at the instance attribute, if it has not been set, it falls back to the class level attribute, but if the instance attribute is set (by any means) (ex a tool that takes a radio group as part of its __init__ which has the use-case of building a bunch of radio buttons to show one line each from several collections of lines) this this code will work as expected and will do no harm to the existing code.

Done, seems fair

Can you put docstrings on these with the expected types?

done

This name is a bit too ambiguous, can it be process_tool_trigger or emit_tool_trigger or something like that? (I don't have my head around the code enough to have a good idea yet, but I don't like this name)

Agree, what do you think of just trigger_tool? it does both, emit the event and trigger the tool

👍 to 'trigger_tool'

done

This isn't lined up with the underscores below

done