Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand cycler() signature. #14

Merged
merged 4 commits into from
Aug 21, 2015
Merged

Conversation

WeatherGod
Copy link
Member

Closes #12 . If you don't like the idea of breaking the API, we can easily add in support for a single pair of positional args, and keep just about everything else.

Btw, I didn't have time to look up how to display and reference multiple call signatures in numpydoc format, so it is probably "wrong".

cycler(label1=itr1[, label2=iter2[, ...]])

Form 1 simply copies a given `Cycler` object.
Form 2 composes a `Cycler` as an inner product of the
Copy link
Member

Choose a reason for hiding this comment

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

"inner product" might not mean much to many docstring readers here. Maybe add something like, "All iterables are cycled simultaneously."

@WeatherGod
Copy link
Member Author

On my way home, I realize a situation where we would want to keep the two argument form. Technically speaking, Cyclers can use any hashable as keys, but only certain kinds of strings can be keyword arguments. The two-argument form enables situations where the key can't be a keyword argument. We might also want to change the repr back to the two-argument form so that it will always be valid syntax.

@efiring
Copy link
Member

efiring commented Aug 20, 2015

True--but is there a significant cycler use case for keys that are not valid keywords?

@WeatherGod
Copy link
Member Author

Maybe, maybe not (I haven't delved into my work project enough to determine
this). But if we want to exclude that, then we should perform checking on
the keys to make sure they are valid identifiers.

On Thu, Aug 20, 2015 at 7:37 PM, Eric Firing notifications@github.com
wrote:

True--but is there a significant cycler use case for keys that are not
valid keywords?


Reply to this email directly or view it on GitHub
#14 (comment).

if not isinstance(args[0], Cycler):
raise TypeError("If only one positional argument given, it must "
" be a Cycler instance.")
return copy.copy(args[0])
Copy link
Member

Choose a reason for hiding this comment

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

Probably want to do a keep copy, I can in to some really weird bugs earlier due to underlying dict objects getting shared. I think this will play badly with the re-labeling functionality.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want me to use copy.deepcopy() everywhere, or just here?

@tacaswell
Copy link
Member

Can we keep the 2 positional args as valid input as well? I see the advantage to this (and going **kwargs is way better than expecting an even number of args), but for reasons I can't articulate I like

cycler('c', 'rgb')

better than

cycler(c='rgb')

even though the first is is more typing.

One use-case for non-string keys is if someone is using Enums as keys. Another, possible too-cute, use-case is using functions as keys.

I am so-so to 👎 with breaking API. It has only been out for a month or two, but we have some external users (@danielballan is using this in our day job) and there is at least one plausible use case for non-string keys.

👍 on adding the implicit + for many kwargs.

👍 on changing the repr back to the cycler(a, b) version.

@danielballan
Copy link
Contributor

Maybe I'm just used to the current API, but I also like two args, and I am
using cycles with nonstring objects as keys.
On Thu, Aug 20, 2015 at 10:51 PM Thomas A Caswell notifications@github.com
wrote:

Can we keep the 2 positional args as valid input as well? I see the
advantage to this (and going **kwargs is way better than expecting an
even number of args), but for reasons I can't articulate I like

cycler('c', 'rgb')

better than

cycler(c='rgb')

even though the first is is more typing.

One use-case for non-string keys is if someone is using Enums as keys.
Another, possible too-cute, use-case is using functions as keys.

I am so-so to [image: 👎] with breaking API. It has only been out for a
month or two, but we have some external users (@danielballan
https://github.com/danielballan is using this in our day job) and there
is at least one plausible use case for non-string keys.

[image: 👍] on adding the implicit + for many kwargs.

[image: 👍] on changing the repr back to the cycler(a, b) version.


Reply to this email directly or view it on GitHub
#14 (comment).

@WeatherGod
Copy link
Member Author

Ok, I brought back the 2-arg form of the call signature, and even modified some unit tests to use labels that can't be used as a keyword argument (which caught a couple things). Also updated the docs a bit per comments.


assert_raises(ValueError, cycler, 'c', c2 + c3)
assert_raises(ValueError, cycler, c=c2+c3)
Copy link
Member

Choose a reason for hiding this comment

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

should have both of these tests 🐑

Copy link
Member

Choose a reason for hiding this comment

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

never mind

@tacaswell tacaswell closed this Aug 21, 2015
@tacaswell tacaswell reopened this Aug 21, 2015
tacaswell added a commit that referenced this pull request Aug 21, 2015
ENH/API: Expand cycler() signature
@tacaswell tacaswell merged commit cd0e03c into matplotlib:master Aug 21, 2015
@tacaswell
Copy link
Member

thanks @WeatherGod

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants