Add a convenience class to sync traitlet attributes #4901

jdfreder · 2014-01-28T17:54:03Z

I'm opening this PR for @jasongrout

Basically this adds an easy way to synchronize two traitlets.

minrk · 2014-01-28T17:55:13Z

IPython/utils/traitlets.py

+    @contextlib.contextmanager
+    def _busy_updating(self):
+        self.updating = True
+        yield


this should be

try: yield finally: self.updating = False

to protect the state against exceptions.

jdfreder · 2014-01-28T17:58:06Z

@minrk thanks, changes made

jasongrout · 2014-01-28T18:30:23Z

IPython/utils/traitlets.py

+    """
+    updating = False
+    def __init__(self, *args):
+


we probably should delete the extra line here.

jasongrout · 2014-01-28T18:30:37Z

Thanks for opening this!

jasongrout · 2014-01-28T18:41:21Z

Issue #4659 should probably be aware of this PR.

minrk · 2014-01-28T18:45:26Z

Yes, the external traitlets effort is going to start over with recent changes after we cut 2.0, and do some cleanup in IPython.config.

jdfreder · 2014-01-28T19:30:09Z

Thanks for opening this!

Thank you for writing the code :)

ellisonbg · 2014-01-29T19:15:47Z

Can you write some simple tests for this in test_traitlets?

ellisonbg · 2014-01-29T19:16:53Z

My main question is to the naming of things. Previously, we had used the language "bind" for this type of thing. Do we like "connect" better than "bind"? And being a verb, I think that either of these should be lowercase (even though a class).

jdfreder · 2014-02-03T17:13:48Z

My vote would be for Bind because it's shorter; however, without the capital B I would forget that it was a class and I wouldn't think to construct it.

ellisonbg · 2014-02-03T20:05:39Z

But wouldn't just calling it bind(...) construct it?

On Mon, Feb 3, 2014 at 9:13 AM, Jonathan Frederic
notifications@github.comwrote:

My vote would be for Bind because it's shorter; however, without the
capital b I would forget that it was a class and I would never construct
it.

Reply to this email directly or view it on GitHubhttps://github.com//pull/4901#issuecomment-33976627
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

takluyver · 2014-02-03T20:46:44Z

IPython/utils/traitlets.py

+            return
+        with self._busy_updating():
+            for obj,attr in self.objects:
+                setattr(obj, attr, new)


This will re-set the trait on the object that changed it, which means any other callbacks it has registered will fire twice. Do we want to exclude that case in order to avoid this?

Yes, I think you are right - we should figure out how to fix this.

Nice catch! I will use a closure to fix this...

obj1, objN undefined was causing the test to fail.

jdfreder · 2014-02-03T21:38:10Z

@takluyver I added the code to prevent double fires and a test to make sure it works as expected.

Add a convenience class to sync traitlet attributes

jasongrout · 2014-02-04T01:47:30Z

I prefer 'friendly' verbs like 'connect' and 'share' over verbs with slightly negative connotation like 'bind'. For the same reason, I prefer 'interact' over mathematica's name, 'manipulate'.

But I also realize that UI conventions use 'bind' a lot, so I'm -0 on 'bind' and +0 on 'connect' or 'share'.

jasongrout · 2014-02-04T01:53:03Z

Looking at http://thesaurus.com/browse/connect, 'consociate' would be an interesting choice. More seriously, though, 'equate', or from http://thesaurus.com/browse/share, 'distribute'.

takluyver · 2014-02-04T01:59:33Z

equate is possible, but I don't especially prefer it over bind. I don't think distribute conveys what this is doing well.

jasongrout · 2014-02-04T02:09:03Z

A few more from http://thesaurus.com/browse/attach: 'link', 'fasten', 'join', 'attach', 'couple', or further searching: 'lock', 'fuse', 'identify',

minrk · 2014-02-04T03:19:47Z

link makes sense to me, but I don't have strong feelings.

jasongrout · 2014-02-04T03:27:42Z

I have strong enough feelings about it to submit a PR to change things to 'link', if it would be accepted.

ellisonbg · 2014-02-04T05:27:56Z

In general I am in agreement with picking names that don't have negative
connotations: interact is much better than manipulate. But in the case of
bind I think the negative connotations are not so strong that they outweigh
using a simple, clear term that is used everywhere in UI work to mean this
exactly.

On Mon, Feb 3, 2014 at 7:27 PM, Jason Grout notifications@github.comwrote:

I have strong enough feelings about it to submit a PR to change things to
'link', if it would be accepted.

Reply to this email directly or view it on GitHubhttps://github.com//pull/4901#issuecomment-34027543
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

jasongrout · 2014-02-04T05:30:41Z

Yep, so I'm still +0 on something like link and -0 on bind. I'm motivated enough to open a PR if it will be accepted, but not motivated enough to push hard for a change...

ellisonbg · 2014-02-04T05:35:07Z

Let's get a read from @minrk @takluyver and @fperez tomorrow on this

On Mon, Feb 3, 2014 at 9:30 PM, Jason Grout notifications@github.comwrote:

Yep, so I'm still +0 on something like link and -0 on bind. I'm motivated
enough to open a PR if it will be accepted, but not motivated enough to
push hard for a change...

Reply to this email directly or view it on GitHubhttps://github.com//pull/4901#issuecomment-34032051
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

takluyver · 2014-02-04T18:16:58Z

No strong feelings between link and bind. Some of the others suggested (distribute, lock) seem clearly wrong, but I don't think those are in serious consideration.

minrk · 2014-02-04T18:48:36Z

I still like link. Maybe it's because of their meanings when talking about sockets, but bind/connect indicate asymmetry to me.

jasongrout · 2014-02-04T19:19:23Z

@takluyver: I was brainstorming through a thesaurus---not all suggestions are equally appropriate!

+1 to minrk's thinking about being symmetrical.

ellisonbg · 2014-02-04T21:36:07Z

I do agree that bind is a bit one way. Probably preferring connect or link.
Share has too many non-technical connotations.

On Tue, Feb 4, 2014 at 11:19 AM, Jason Grout notifications@github.comwrote:

@takluyver https://github.com/takluyver: I was brainstorming through a
thesaurus---not all suggestions are equally appropriate!

+1 to minrk's thinking---that's the other thing I liked about connect or
share: it was symmetrical.

Reply to this email directly or view it on GitHubhttps://github.com//pull/4901#issuecomment-34096040
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

ellisonbg · 2014-02-04T21:36:39Z

And probably prefer connect over link as it is only a verb.

On Tue, Feb 4, 2014 at 1:36 PM, Brian Granger ellisonbg@gmail.com wrote:

I do agree that bind is a bit one way. Probably preferring connect or
link. Share has too many non-technical connotations.

On Tue, Feb 4, 2014 at 11:19 AM, Jason Grout notifications@github.comwrote:

@takluyver https://github.com/takluyver: I was brainstorming through a
thesaurus---not all suggestions are equally appropriate!

+1 to minrk's thinking---that's the other thing I liked about connect or
share: it was symmetrical.

Reply to this email directly or view it on GitHubhttps://github.com//pull/4901#issuecomment-34096040
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

jasongrout · 2014-02-04T21:55:22Z

Though the connect/link function does return an object, which you can then use to break the connection/link later. So link(...) (verb) would return the link object (noun), by which you could later break the connection.

minrk · 2014-02-05T01:01:05Z

As @jasongrout points out, bonus points to link for being both a noun and a verb, since it is technically an object. So are we settled on link, then?

ellisonbg · 2014-02-05T01:02:13Z

I am reasonably fine with that. I did ping @fperez in an email to see what he thinks. I like his eye for these things, but we can go ahead if there is consensus.

fperez · 2014-02-05T21:10:28Z

+1 for link: the symmetry argument is a good one, and from reading the code, I actually think it expresses the behavior best of all the proposed names. Bind is often used either in connect/bind asymmetric situations, or as in bindings (language wrappers). From reading what the code actually does, I find link to be a very natural description.

For example, many UIs for image processing that want to link two attributes to change in sync (such as resizing an image when you want to preserve its aspect ratio), use a little chain link icon to illustrate that:

That makes me really like 'link' for this. If @jasongrout is willing to do the PR work, that would be great.

jasongrout · 2014-02-07T21:02:05Z

Done. See #5060.

Add a convenience class to sync traitlet attributes

minrk reviewed Jan 28, 2014
View reviewed changes

jasongrout reviewed Jan 28, 2014
View reviewed changes

IPython/utils/traitlets.py

"""

updating = False

def __init__(self, *args):

Copy link

Member

jasongrout Jan 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably should delete the extra line here.

takluyver reviewed Feb 3, 2014
View reviewed changes

Jason Grout and others added 8 commits February 3, 2014 12:48

Add a convenience class to sync traitlet attributes

49433e0

Added try/finally

034be0d

Removed extra space

f905c11

Skip the doc tests for Connect,

295b609

obj1, objN undefined was causing the test to fail.

Added Connect tests

dff1f71

fixes to make Connect pass tests

3e73815

s/Connect/bind

0eaafca

Use closure to prevent traitlet callbacks from firing twice.

466a0e3

ellisonbg added a commit that referenced this pull request Feb 3, 2014

Merge pull request #4901 from jdfreder/jason_traitlets

7dbba6e

Add a convenience class to sync traitlet attributes

ellisonbg merged commit 7dbba6e into ipython:master Feb 3, 2014

jasongrout mentioned this pull request Feb 7, 2014

Change 'bind' to 'link' #5060

Merged

jdfreder deleted the jason_traitlets branch March 10, 2014 18:42

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request ipython#4901 from jdfreder/jason_traitlets

666c961

Add a convenience class to sync traitlet attributes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a convenience class to sync traitlet attributes #4901

Add a convenience class to sync traitlet attributes #4901

jdfreder commented Jan 28, 2014

minrk Jan 28, 2014

jdfreder commented Jan 28, 2014

jasongrout Jan 28, 2014

jasongrout commented Jan 28, 2014

jasongrout commented Jan 28, 2014

minrk commented Jan 28, 2014

jdfreder commented Jan 28, 2014

ellisonbg commented Jan 29, 2014

ellisonbg commented Jan 29, 2014

jdfreder commented Feb 3, 2014

ellisonbg commented Feb 3, 2014

takluyver Feb 3, 2014

ellisonbg Feb 3, 2014

jdfreder Feb 3, 2014

jdfreder commented Feb 3, 2014

jasongrout commented Feb 4, 2014

jasongrout commented Feb 4, 2014

takluyver commented Feb 4, 2014

jasongrout commented Feb 4, 2014

minrk commented Feb 4, 2014

jasongrout commented Feb 4, 2014

ellisonbg commented Feb 4, 2014

jasongrout commented Feb 4, 2014

ellisonbg commented Feb 4, 2014

takluyver commented Feb 4, 2014

minrk commented Feb 4, 2014

jasongrout commented Feb 4, 2014

ellisonbg commented Feb 4, 2014

ellisonbg commented Feb 4, 2014

jasongrout commented Feb 4, 2014

minrk commented Feb 5, 2014

ellisonbg commented Feb 5, 2014

fperez commented Feb 5, 2014

jasongrout commented Feb 7, 2014

Add a convenience class to sync traitlet attributes #4901

Add a convenience class to sync traitlet attributes #4901

Conversation

jdfreder commented Jan 28, 2014

minrk Jan 28, 2014

Choose a reason for hiding this comment

jdfreder commented Jan 28, 2014

jasongrout Jan 28, 2014

Choose a reason for hiding this comment

jasongrout commented Jan 28, 2014

jasongrout commented Jan 28, 2014

minrk commented Jan 28, 2014

jdfreder commented Jan 28, 2014

ellisonbg commented Jan 29, 2014

ellisonbg commented Jan 29, 2014

jdfreder commented Feb 3, 2014

ellisonbg commented Feb 3, 2014

takluyver Feb 3, 2014

Choose a reason for hiding this comment

ellisonbg Feb 3, 2014

Choose a reason for hiding this comment

jdfreder Feb 3, 2014

Choose a reason for hiding this comment

jdfreder commented Feb 3, 2014

jasongrout commented Feb 4, 2014

jasongrout commented Feb 4, 2014

takluyver commented Feb 4, 2014

jasongrout commented Feb 4, 2014

minrk commented Feb 4, 2014

jasongrout commented Feb 4, 2014

ellisonbg commented Feb 4, 2014

jasongrout commented Feb 4, 2014

ellisonbg commented Feb 4, 2014

takluyver commented Feb 4, 2014

minrk commented Feb 4, 2014

jasongrout commented Feb 4, 2014

ellisonbg commented Feb 4, 2014

ellisonbg commented Feb 4, 2014

jasongrout commented Feb 4, 2014

minrk commented Feb 5, 2014

ellisonbg commented Feb 5, 2014

fperez commented Feb 5, 2014

jasongrout commented Feb 7, 2014