Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
removing name argument from tools#9966
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Have you tested this@fariza? As far as I can see the code will fail. |
Yes I tested it. Why do you say it will fail? …On Dec 10, 2017 1:47 PM, "OceanWolf" ***@***.***> wrote: Have you tested this@fariza <https://github.com/fariza>? As far as I can see the code will fail. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9966 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABa86VUBgglw4nw0fsrJe8HwQVZOYn0nks5s_CchgaJpZM4Q8XYn> . |
Ahh sorry, my mistake, I missed something. However you need to change the docstring of |
Done |
@OceanWolf@anntzer can you help merge? |
lib/matplotlib/backend_managers.py Outdated
| self.data = data | ||
| class ToolTriggerEvent(ToolEvent): | ||
| """Event to inform that a tool has been triggered""" | ||
| def __init__(self, name, sender, tool, canvasevent=None, data=None): | ||
| ToolEvent.__init__(self, name, sender, tool, data) | ||
| def __init__(self, name, sender, tool, tool_name, canvasevent=None, data=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This line fails PEP8 I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It does, you just have to look at the Travis build that failed because of this PEP8 error. What's the point in having Travis if we don't use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Don't worry! I used Travis. I just wasn't 100% sure I'd remembered the line right, was too lazy to go back and check it, and was 99.9% sure@fariza could manage to figure it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ahh, okay. I over reacted a bit though, I got an email to say that this PR had been approved, and I mistook that to mean it had been merged. I haven't got used to this new review system yet...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No problem! I didn't approve it - I don't really understand it.
Any other comments? |
lib/matplotlib/backend_bases.py Outdated
| tool :tool_like | ||
| The tool to add, see `ToolManager.get_tool`. | ||
| tool_name :str | ||
| Name of the the tool to add, see `ToolManager.get_tool`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Double 'the'
@QuLogic docstring corrected |
@OceanWolf can you help merge this? |
bump |
| """ | ||
| Adds a tool to this container | ||
| Parameters | ||
| ---------- | ||
| tool : tool_like | ||
| The tool to add, see `ToolManager.get_tool`. | ||
| tool_name : str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is an API change, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As are the added args below? Not sure if they are a problem, but prob needs an Api change note
@fariza f you have the stomach for a rebase I'll merge this on the merits of@anntzer's review. I don't think@OceanWolf will get a chance to review this... |
@jklymak I will give it a try. If I find it too hard, would you be willing to accept a new one with the same functionality? |
@fariza Whatever works best for you. At some point we need to return to these MEPs and see if they are something that is really wanted. MEP 27 is actually a huge deal, and at the very least@anntzer was skeptical. I'm ignorantly skeptical because I don't understand what problems these are trying to solve and whether they are worth the maintenance burden. We are at the beginning of 3.4, and its not foolish to consider 3.5 or 4.0, so if there is still enthusiasm for these ideas it is a reasonable time to agitate. I'm going to add MEP 27 and 22 to this weeks dev call. You are more than welcome to join if you like. I think a lot of enthusiasm and effort went into these and in my opinion you (and@OceanWolf) deserve some finality on whether this should still go ahead. |
@jklymak sure, I'll like to join this weeks dev call |
@fariza Judging from the code changed with this PR I can't imagine a rebase exists as that difficult as the PR mostly touches the new files we created. |
@jklymak I did the rebase, wasn't as hard and painful as I imagined (you were right@OceanWolf ) |
The docs need to build, and I still think this needs an API note? |
@jklymak added the api change notes. (not sure it's the format or wording that we use for that) |
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Remove name from tools.
This is part of the work on#9941
PR Checklist