removing name argument from tools #9966

fariza · 2017-12-10T04:20:36Z

PR Summary

Remove name from tools.
This is part of the work on #9941

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

OceanWolf · 2017-12-10T18:46:49Z

Have you tested this @fariza? As far as I can see the code will fail.

fariza · 2017-12-10T22:22:55Z

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> .

OceanWolf · 2017-12-10T22:59:13Z

Ahh sorry, my mistake, I missed something. However you need to change the docstring of ToolContainerBase.add_tool.

fariza · 2017-12-11T02:23:58Z

Done

fariza · 2017-12-13T14:41:55Z

@OceanWolf @anntzer can you help merge?

jklymak · 2017-12-13T22:34:46Z

lib/matplotlib/backend_managers.py

        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):


This line fails PEP8 I think

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.

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.

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...

No problem! I didn't approve it - I don't really understand it.

fariza · 2017-12-14T14:17:29Z

Any other comments?

QuLogic · 2017-12-14T21:32:30Z

lib/matplotlib/backend_bases.py

-        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`.


Double 'the'

fariza · 2017-12-15T15:55:17Z

@QuLogic docstring corrected

fariza · 2017-12-21T13:35:04Z

@OceanWolf can you help merge this?

fariza · 2018-01-04T22:06:23Z

bump

jklymak · 2019-02-09T22:59:16Z

lib/matplotlib/backend_bases.py

        """
        Adds a tool to this container

        Parameters
        ----------
-        tool : tool_like
-            The tool to add, see `ToolManager.get_tool`.
+        tool_name : str


This is an API change, isn't it?

As are the added args below? Not sure if they are a problem, but prob needs an Api change note

jklymak · 2020-07-15T23:00:15Z

@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...

fariza · 2020-07-16T14:17:47Z

@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?

jklymak · 2020-07-16T14:28:24Z

@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.

fariza · 2020-07-16T14:39:43Z

@jklymak sure, I'll like to join this weeks dev call

OceanWolf · 2020-07-16T14:43:07Z

@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.

fariza · 2020-07-17T03:08:10Z

@jklymak I did the rebase, wasn't as hard and painful as I imagined (you were right @OceanWolf )
If you want, wait until the call to decide if we merge it or not

jklymak · 2020-07-17T04:20:48Z

The docs need to build, and I still think this needs an API note?

fariza · 2020-07-17T22:07:11Z

@jklymak added the api change notes. (not sure it's the format or wording that we use for that)

github-actions · 2023-04-27T01:54:02Z

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.

fariza mentioned this pull request Dec 10, 2017

Explicit tool registration. #9941

Closed

6 tasks

fariza requested review from OceanWolf and anntzer December 11, 2017 14:01

anntzer approved these changes Dec 13, 2017

View reviewed changes

jklymak reviewed Dec 13, 2017

View reviewed changes

QuLogic reviewed Dec 14, 2017

View reviewed changes

fariza mentioned this pull request Jan 4, 2018

MEP22 implementation for QT backend #9934

Merged

6 tasks

jklymak added this to the v3.0 milestone Mar 16, 2018

jklymak modified the milestones: v3.0, v3.1 Jul 9, 2018

jklymak added status: needs rebase topic: toolbar labels Feb 9, 2019

jklymak reviewed Feb 9, 2019

View reviewed changes

jklymak modified the milestones: v3.1.0, v3.2.0 Feb 25, 2019

tacaswell modified the milestones: v3.2.0, v3.3.0 Aug 19, 2019

QuLogic modified the milestones: v3.3.0, v3.4.0 Apr 30, 2020

jklymak removed the request for review from OceanWolf July 15, 2020 23:00

fariza and others added 5 commits July 16, 2020 21:43

removing name argument from tools

8131760

docstring correction

ba1ab48

pep8 fix

a107967

fix docstring

7a02d51

docstring corrections

ef686fc

fariza force-pushed the remove-name-tools branch from 097c9ab to ef686fc Compare July 17, 2020 02:56

remove unused method

375c2f0

api changes

8695315

jklymak marked this pull request as draft August 24, 2020 14:19

QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 21, 2021

QuLogic modified the milestones: v3.5.0, v3.6.0 Aug 18, 2021

timhoffm modified the milestones: v3.6.0, unassigned Apr 30, 2022

story645 modified the milestones: unassigned, needs sorting Oct 6, 2022

github-actions bot added the status: inactive Marked by the “Stale” Github Action label Apr 27, 2023

Uh oh!

removing name argument from tools #9966

Are you sure you want to change the base?

removing name argument from tools #9966

Uh oh!

Conversation

fariza commented Dec 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

OceanWolf commented Dec 10, 2017

Uh oh!

fariza commented Dec 10, 2017 via email

Uh oh!

OceanWolf commented Dec 10, 2017

Uh oh!

fariza commented Dec 11, 2017

Uh oh!

fariza commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fariza commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fariza commented Dec 15, 2017

Uh oh!

fariza commented Dec 21, 2017

Uh oh!

fariza commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commented Jul 15, 2020

Uh oh!

fariza commented Jul 16, 2020

Uh oh!

jklymak commented Jul 16, 2020

Uh oh!

fariza commented Jul 16, 2020

Uh oh!

OceanWolf commented Jul 16, 2020

Uh oh!

fariza commented Jul 17, 2020

Uh oh!

jklymak commented Jul 17, 2020

Uh oh!

fariza commented Jul 17, 2020

Uh oh!

github-actions bot commented Apr 27, 2023

Uh oh!

Uh oh!

fariza commented Dec 10, 2017 •

edited

Loading