adding default toggled state for toggle tools

[fariza]

So far toggle tools start their life in the untoggled state, most of the time this is what we want but sometimes it is desirable to initialize a ToogleTool in a toggled state without firing the tool_trigger_xxx event

With the changes in this PR, the initial state can be changed by class attribute during Tool definition default_toggle = True or at the moment of adding the tool to the Toolmanager toolmanager.add_tool('Name', ToolCls, toggle=True)

[fariza]

What is the standard to document optional kwargs? like toggled in this particular case?

[tacaswell]

at least wrap it it double back ticks so that rst does not choke on opened, but not closed emphasis

[tacaswell]

The docs are passing which makes me think these are not actually in the docs?

[fariza]

It is building the docs, I got that from a numpydoc example somewhere and it didn't have quotes around it

[tacaswell]

Interesting, I spent some time fighting with things like that breaking the doc build on some day-job projects.....

[fariza]

Just in case I added the double back tick

[fariza]

I don't think the fail is related to these changes

[fariza]

@OceanWolf any thoughts on this?

[fariza]

@OceanWolf this one is the other that needs a little bit of your attention.
But it doesn't affect MEP27,

[OceanWolf]

Just gave it a quick look and it looks good to me. Will give it another look later.

When I first started reading I thought you might go down the object overriding class attribute approach, as far as I see both methods work fine.

[fariza]

I rebased to eliminate conflicts and upgraded the example to use the initial toggled state
I transformed the tool from Hide to Show

[QuLogic]

You have some PEP8 errors; AppVeyor failure appears to be unrelated.

[fariza]

@QuLogic pep8 erros corrected

[fariza]

ready to merge?

[WeatherGod]

@fariza, I just noticed that this PR was self-merged. I don't want to make a huge deal about it, as these changes are non-controversial and there was a lack of responses to the PR. The matplotlib organization does not enforce a ban on self-merges, but it is highly discouraged. We don't enforce such a ban because there are emergency situations where it is the correct thing to do (getting TravisCI happy again because pyparsing put out a bad release is an example).

What I would suggest you do next time you get a lack of responses to your PR is to ping the PR. If you still don't get responses, then raise the issue on the gitter chat room or on the mailing list. The lack of responses could simply be due to vacation schedules or other things going on.

As for this PR, if I had actually spent time reviewing it at the time (sorry for not doing so), I would have asked for some tests, if it was possible. The coverage went down because of this PR, which is less than ideal. If tests are possible, then could you please create a PR to include them, please?

[fariza]

Sorry for the self merge.

I understand the policy and be sure it won't happen again.

Regarding the tests, at this moment we don't have the infrastructure to
test this kind of things.
What I was planning is to wait for Mep27 to land and stabilize. Then add a
complete "virtual backend" that will allow to test all the tools and
managers. In this case the coverage goes down just because of extra lines
of code but it is not an actual reduction.

Federico

On Aug 16, 2016 5:12 PM, "Benjamin Root" notifications@github.com wrote:

@fariza https://github.com/fariza, I just noticed that this PR was
self-merged. I don't want to make a huge deal about it, as these changes
are non-controversial and there was a lack of responses to the PR. The
matplotlib organization does not enforce a ban on self-merges, but it is
highly discouraged. We don't enforce such a ban because there are emergency
situations where it is the correct thing to do (getting TravisCI happy
again because pyparsing put out a bad release is an example).

What I would suggest you do next time you get a lack of responses to your
PR is to ping the PR. If you still don't get responses, then raise the
issue on the gitter chat room or on the mailing list. The lack of responses
could simply be due to vacation schedules or other things going on.

As for this PR, if I had actually spent time reviewing it at the time
(sorry for not doing so), I would have asked for some tests, if it was
possible. The coverage went down because of this PR, which is less than
ideal. If tests are possible, then could you please create a PR to include
them, please?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6407 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABa86WZXYz-HkxVxzc6jcj-bLiZtxzhEks5qgifAgaJpZM4Icle9
.

[OceanWolf]

@WeatherGod Ahh, so the coveralls relates to how much of the codebase we have covered by unit-tests?

I kind of understood that that policy existed, but do we have a guide somewhere outlining these kinds of things for the less obvious rules/guidelines?

As for fariza's point, yes, unit tests for backends becomes very difficult due to their graphical nature. In MEP27 I added the template classes, so maybe we can do our testing around that?

I had hoped that pgi would solve our testing woes with gtk3, but alas it works well enough for basic testing, but not good enough for production yet (very buggy). Maybe I can take another look at pgi, improve my gdb debugging skills with python to learn where/how it segfaults, and get that up and running ASAP.