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

Validating, __init__-based assoc. #124

Closed
wants to merge 3 commits into from

Conversation

@Tinche
Copy link
Member

@Tinche Tinche commented Dec 12, 2016

Here is an __init__-based assoc implementation that handles private attributes.

Using the same three classes from #116, I've done benchmark runs for all-public and all-private attributes on the classes. Private attributes add a slight overhead now, the old implementation was unaffected. Underscores in front of class names indicate private attributes were used.

                A                       B                      C
--------------------------------------------------------------------------------------
Old assoc:      10.6  us +- 0.3  us     12.7  us +- 0.2  us    14.8  us +- 0.3  us
New assoc:       2.07 us +- 0.06 us      5.23 us +- 0.13 us     8.83 us +- 0.28 us


                _A                      _B                     _C
--------------------------------------------------------------------------------------
Old assoc:      10.5  us +- 0.2  us     12.6  us +- 0.2 us     14.9 us +- 0.3 us
New assoc:       2.58 us +- 0.07 us      6.16 us +- 0.12 us    10.7 us +- 0.2 us

I'm pretty sure we could handle the users supplying both private and public attribute names (i.e. both setter and init syntax) with a tiny performance cost, but is it worth it? First of all,

There should be one-- and preferably only one --obvious way to do it.

second, could there be corner cases? I can't think of any (

class A:
    _a = attr.ib()
   __a = attr.ib()

isn't one of them, fortunately :) but maybe there are some?

Doc change and CHANGELOG entry to come when we're satisfied with the implementation.

Tinche added 2 commits Dec 12, 2016
@codecov-io
Copy link

@codecov-io codecov-io commented Dec 12, 2016

Current coverage is 100% (diff: 100%)

Merging #124 into master will not change coverage

@@           master   #124   diff @@
====================================
  Files           8      8          
  Lines         502    511     +9   
  Methods         0      0          
  Messages        0      0          
  Branches      110    112     +2   
====================================
+ Hits          502    511     +9   
  Misses          0      0          
  Partials        0      0          

Powered by Codecov. Last update 49e8470...ec4803b

@hynek
Copy link
Member

@hynek hynek commented Dec 19, 2016

Nah we don’t have to support both. I had a slightly different approach in mind (which I honestly forgot :)) which would’ve given it us for free. Other than that I don’t have much to add. The performance gains look awesome!

We can always add support for underscore names if enough people complain. But removing is much harder.

@Tinche
Copy link
Member Author

@Tinche Tinche commented Dec 19, 2016

Just to clarify, this implementation supports underscore names, i.e. the setter syntax.

(For backwards compatibility.)

@hynek
Copy link
Member

@hynek hynek commented Dec 19, 2016

Oh, so it doesn’t support the __init__ syntax?

@Tinche
Copy link
Member Author

@Tinche Tinche commented Dec 19, 2016

No, this is setter only. I figured from the issue this is what you wanted. I mean, this implementation is a little more complex and a little slower than the init syntax, so if that's what you want I can revert it.

@hynek
Copy link
Member

@hynek hynek commented Dec 19, 2016

No it’s alright. Backward compatibility FTW. Please finish up. :)

@hynek
Copy link
Member

@hynek hynek commented Dec 21, 2016

I’ll have to meditate on the backward-incompatible part.

To me, this is a bug fix because I think everyone would expect it to behave this way. The point about __init__ is true tho. Technically we’d have to announce it now and wait for a year though.

Hmmmm.

@hynek
Copy link
Member

@hynek hynek commented Dec 21, 2016

In any case:

  • we’ll have to add a note to the assoc docstring about the init situation
  • you have to stop using Markdown in rST :)
  • it shouldn’t be called attrs-style init but described what it actually means instead. It looks like the init has to be literally written by attrs like this.
@glyph
Copy link
Contributor

@glyph glyph commented Dec 21, 2016

The usual guidance is in Twisted when people start fretting about changed behavior is "new names are cheaper than you think". Instead of assoc, have assoc2; deprecate assoc. Then, come up with a good name for assoc2.

FWIW I have always hated assoc as a name; it's an appeal to lisp-nerdery and nobody knows how to pronounce it. I think clone expresses this functionality in terms that Python peeps are more likely to use. But, even if it's just assoc in a different namespace, that would be a way to make a clean break.

@glyph
Copy link
Contributor

@glyph glyph commented Dec 21, 2016

( I should also note, though, that this is a good change. I was just reviewing a PR on https://github.com/rackerlabs/otter where I was lamenting the odd behavior of assoc and its inconsistencies with __init__. )

@Tinche
Copy link
Member Author

@Tinche Tinche commented Jan 6, 2017

I'm still with the inlaws for the holidays and away from my home computer. I've been thinking of the name, though. I think Glyph is right and we should leave assoc as is, deprecate it, and think of a better name for this implementation.

What's actually happening is:

  • a shallow copy is being made
  • with optional changes incorporated

'Copy' or 'clone' are okay but they don't really mesh well with the fact the original object is being changed. In fact, they kinda imply the object is not being changed at all. In my opinion.

How about 'recons', as in 'reconstruct' or 'reconstitute'? We're breaking the instance into parts (asdict), then putting it back together, changed.

>>> attr.recons(obj, x=1)

(attr.reconsitute is too long I think.)

Another idea is attr.reinit. Init evokes the initializer.

@glyph
Copy link
Contributor

@glyph glyph commented Jan 6, 2017

All of the various derivatives of the word "change" - "modify", "mutate", etc, imply the thing itself is changing, as opposed to a new thing coming into existence.

But wait - in that sentence I used the word "derivative" to describe exactly that sort of thing! This might be too cutesy given its overlap with other terms, especially math terms, but derive might be the word we're looking for. If you have A, and you want A', where A' is like A but with some changes, you could say A' is derived from A.

I could see attr.derive(obj, x=1) being idiomatic.

Thoughts?

@hynek
Copy link
Member

@hynek hynek commented Jan 6, 2017

I agree that derive is the first name that I could sort of live with but I'm not happy with it.

There has to be some term that isn't some obscure math/FP lingo and that expresses precisely what we need!?

I really don't want it to be unclear what it's about.

@glyph
Copy link
Contributor

@glyph glyph commented Jan 6, 2017

There has to be some term that isn't some obscure math/FP lingo and that expresses precisely what we need!?

"convolve"? :)

@glyph
Copy link
Contributor

@glyph glyph commented Jan 6, 2017

or perhaps "comprise"?

@hynek
Copy link
Member

@hynek hynek commented Jan 6, 2017

OK one more rule: I don’t have to look it up in a dictionary. ಠ_ಠ

@hynek
Copy link
Member

@hynek hynek commented Jan 6, 2017

evolve wouldn't be too bad but also kind of obscure. :-/

@hynek
Copy link
Member

@hynek hynek commented Jan 6, 2017

FTR, pyrsistent seems to have transform which doesn’t seem too bad?

@glyph
Copy link
Contributor

@glyph glyph commented Jan 6, 2017

FTR, pyrsistent seems to have transform which doesn’t seem too bad?

Except for the fact that "transform" also means "change" in the "mutate the existing object" sort of way.

@glyph
Copy link
Contributor

@glyph glyph commented Jan 6, 2017

evolve wouldn't be too bad but also kind of obscure. :-/

Actually this might be the best bet. Consider that even the average 7-year-old knows that Pokémon have "evolutions".*

*: except of course what pokémon actually have are metamorphoses, which are destructive changes, but as a real species "evolves" it involves creating lots of new instances

@hynek
Copy link
Member

@hynek hynek commented Jan 6, 2017

I was rather thinking about evolution which usually happens over generations. Humans evolved from ape. There was no ape who suddenly liked computers.

@hynek hynek modified the milestone: 17.0 Jan 6, 2017
@hynek
Copy link
Member

@hynek hynek commented Jan 11, 2017

So I believe we’ve agreed on deprecating assoc as it is and implementing a new evolve(), correct?

@Tinche
Copy link
Member Author

@Tinche Tinche commented Jan 11, 2017

Alright, so we're settled on evolve? I prefer derive, but the boss has the final say of course :)

I'll put together a new PR soon-ish. Are we deprecating assoc though?

@glyph
Copy link
Contributor

@glyph glyph commented Jan 12, 2017

So I believe we’ve agreed on deprecating assoc as it is and implementing a new evolve(), correct?

Yay! Evolvulation!

@hynek
Copy link
Member

@hynek hynek commented Jan 12, 2017

I can live with derive too...I guess I'll have to check a thesaurus or something. :-/

@glyph
Copy link
Contributor

@glyph glyph commented Jan 12, 2017

Sorry, I didn't mean that to seem critical of the choice of "evolve", I just found the quote evocative :).

evolve is great. If you want to consult a thesaurus, don't let me stop you, but don't go back to derive purely on my account.

@hynek
Copy link
Member

@hynek hynek commented Jan 12, 2017

I like both for different reasons. :|

@Tinche
Copy link
Member Author

@Tinche Tinche commented Jan 12, 2017

Working on this a little.

The docs state:

If there will ever be a need to break compatibility, it will be announced in the Changelog and raise a DeprecationWarning for a year (if possible) before it’s finally really broken.

Ok, let's raise a DeprecationWarning, as per the docs.

import warnings
warnings.warn("deprecated", DeprecationWarning)

Nothing happens. Pop over to https://docs.python.org/3.6/library/warnings.html:

By default, Python installs several warning filters, which can be overridden by the command-line options passed to -W and calls to filterwarnings().

  • DeprecationWarning and PendingDeprecationWarning, and ImportWarning are ignored.
    ...

Changed in version 3.2: DeprecationWarning is now ignored by default in addition to PendingDeprecationWarning.

Doesn't this make DeprecationWarning basically useless? Who runs their Python with -W? Not me. In fact I had to Google how to enable warnings at all (it's python -Wd).

@bluetech
Copy link

@bluetech bluetech commented Jan 13, 2017

Regarding the name, IIUC (I have very little experience with the library, so maybe not), this operation is similar to namedtuple._replace, in which case replace might be appropriate.

@bluetech
Copy link

@bluetech bluetech commented Jan 13, 2017

@Tinche Django advises its users to enable warnings when updating, at least: https://docs.djangoproject.com/en/1.10/howto/upgrade-version/#resolving-deprecation-warnings

@hynek
Copy link
Member

@hynek hynek commented Jan 14, 2017

Deprecation Warnings

We should add documentation on them to the backward compatibility page.

Naming

I’m not a fan of replace() because it doesn’t express what it does at all (for namedtuples the copy is implicit because they’re always immutable). But we went with namedtuple lingo before (asdict) so there’s a precedent and I’m not entirely a fan of the others either. :|

So far we have:

  • replace(): established lingo, no semantics in name
  • evolve(): semantics can be guessed but obscure
  • derive(): i’ve heard people calling subclassing as derivation too so that might be confusing.

I’ve spend some time in a thesaurus and found nothing better.

Anyone more 🎨 ?

@glyph
Copy link
Contributor

@glyph glyph commented Jan 14, 2017

If I may adjust my 🚲🏠🎨:

I think you're right about "derive". A subclass is, formally, a derived class. So if, as its original submitter, I can withdraw it from consideration, I will do so.

evolve() is slightly obscure, but anything that isn't obscure will, almost by definition, have an existing, conflicting meaning that will be confusing. I think evolve() is in a sweet spot where it's not too obscure, and the English word matches pretty closely what the programmatic operation is. The only overlapping place I can think of it being used is in genetic programming, and even there, it would make sense in context.

replace() is just awful. It already has numerous existing meanings in Python, all of which imply both mutation and the examination/comparison of a value except for the one in namedtuple (which is really obscure - an underscore method that's documented in stdlib docs? this leaves it in the company of _getframe).

@glyph
Copy link
Contributor

@glyph glyph commented Jan 14, 2017

Doesn't this make DeprecationWarning basically useless?

Yep. Huge mistake on the part of Python core if you ask me. Still hoping they'll reverse it.

Who runs their Python with -W?

Me!

Or, well, not exactly. But trial turns on deprecation warnings by default, and if you drop this gem into your .bashrc while you're developing:

    export PYTHONWARNINGS='all::DeprecationWarning'

you don't need to actually inject the command-line arg.

@bluetech
Copy link

@bluetech bluetech commented Jan 14, 2017

an underscore method that's documented in stdlib docs?

The reason is explained in the namedtuple docs:

To prevent conflicts with field names, the method and attribute names start with an underscore.

In attrs it's a module function so it's not a problem.

@hynek
Copy link
Member

@hynek hynek commented Jan 14, 2017

Yeah namedtuple's underscore methods is one of the reasons why attrs uses functions wherever possible.

@hynek
Copy link
Member

@hynek hynek commented Jan 14, 2017

Tin! Let's evolve attrs!

@Tinche
Copy link
Member Author

@Tinche Tinche commented Jan 20, 2017

Closing in favor of #135.

@Tinche Tinche closed this Jan 20, 2017
@Tinche Tinche deleted the Tinche:feature/assoc-refactor branch Jan 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.