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

Feature/slots #35

Merged
merged 12 commits into from Mar 31, 2016
Merged

Feature/slots #35

merged 12 commits into from Mar 31, 2016

Conversation

@Tinche
Copy link
Member

@Tinche Tinche commented Mar 17, 2016

No description provided.

@codecov-io
Copy link

@codecov-io codecov-io commented Mar 17, 2016

Current coverage is 100.00%

Merging #35 into master will not affect coverage as of 8929cc7

@@            master     #35   diff @@
======================================
  Files            7       7       
  Stmts          343     361    +18
  Branches        80      86     +6
  Methods          0       0       
======================================
+ Hit            343     361    +18
  Partial          0       0       
  Missed           0       0       

Review entire Coverage Diff as of 8929cc7

Powered by Codecov. Updated on successful CI builds.

Loading

foo = None


@attr.s()

This comment has been minimized.

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 17, 2016

On a first scroll a few nits to keep you busy until I have actual time for a review :):

Please:

  1. replace class_ through cls.
  2. use my docstring style consistently (mostly test_slots.py)

Loading

Fix docstrings.
Fix missing parens in a test.
@hynek
Copy link
Member

@hynek hynek commented Mar 21, 2016

OK one more fluff wish: could you please make all asserts to be assert expected == rv? I don’t Yoda in ifs either but I like to have the expected values lined up. :)

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 21, 2016

Just so we don’t forget:

  • document that slots=True replaces the class

Loading

cl_dict = dict(cl.__dict__)
cl_dict['__slots__'] = tuple(ca_list)
for ca_name in ca_list:
# It might not actually be in there, f.e. if using 'these'.

This comment has been minimized.

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 21, 2016

all in all this is exciting :)

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Mar 21, 2016

Pasting the answer link to a review question here so it doesn't get hidden by GitHub:

#35 (comment)

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 22, 2016

A question? Wrong link? Seemed more like a statement of facts. :)

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Mar 22, 2016

Ok, a clarification then. :)

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 22, 2016

Are you waiting for anything from me?

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Mar 22, 2016

Er no, I'm at work currently. Will fix the parametrization decorators tonight. That's all for now right?

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 22, 2016

That and docs. :)

FWIW, you can also use hypothesis for the decorators thing. I tend to introduce it into most of my current projects.

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Mar 22, 2016

Ok, about the docs. Obviously we need to document the slots argument to attr.s() and make_class, but we should probably have a small section somewhere on the benefits and drawbacks of slot classes. Any preference where?

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 22, 2016

I’d say examples. Bring an example and explain a bit. But you don’t have to explain slots completely. Only what’s special about our slots. Feel free to link a general description.

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Mar 22, 2016

Ok, I've added hypothesis in. The tests for py27 and pypy are actually failing for me currently, since hypothesis conditionally depends on enum34, and tox creates virtualenvs with ancient pip versions (1.5.6) which don't know to actually install enum34 for hypothesis. I will admit to not knowing how to fix this, and getting a little pissed off at tox for not having a mechanism to deal with this (or at least a discoverable mechanism).

I'll ping here when there's something to show for the docs, might be a few days.

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 23, 2016

You mean local? Because Travis passes just fine? Try tox -r :)

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Mar 23, 2016

I did try recreating, no dice. I guess my virtualenv package is just obsolete. Still frustrating to not be able to precisely tailor the test environment without hacks :/

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 23, 2016

I use pipsi for tox and keep my virtualenv/pip up2date, yeah. :)

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Mar 28, 2016

Just pushed some docs, for your inspection. Out of curiosity, do you get automatic notifications when I push new stuff?

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 29, 2016

Nope, always ping tickets when you do something. Getting a notification for each push would result in a lot of mails I guess but it would be nice if there’d be a button “notify maintainers” or something. :-/

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 29, 2016

Please do semantic new lines. :)

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Mar 29, 2016

Alright, am I doing it right? :)

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 29, 2016

I'll give more detailed feedback tomorrow but you slightly over-did it. :) No line feeds after commas please. :)

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Mar 29, 2016

Loading


By default, instances of classes have a dictionary for attribute storage.
This wastes space for objects having very few instance variables.
The space consumption can become acute

This comment has been minimized.

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 30, 2016

Getting there! 🎉

Loading

* capitalize bullet items
* replace bullet asterisks with dashes
* indent bullet code fragments
* join some lines
@Tinche
Copy link
Member Author

@Tinche Tinche commented Mar 30, 2016

Alright, I've incorporated your feedback to the best of my abilities :)

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 31, 2016

Change log entry and I think we can merge. :)

Loading

@hynek
Copy link
Member

@hynek hynek commented Mar 31, 2016

Nevermind, I’ll do it myself. Thanks for all the work!

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Mar 31, 2016

My pleasure, thanks for the reviews.

Loading

@hynek hynek merged commit adb08a1 into python-attrs:master Mar 31, 2016
2 checks passed
Loading
hynek added a commit that referenced this issue Mar 31, 2016
hynek added a commit that referenced this issue Mar 31, 2016
@glyph
Copy link
Contributor

@glyph glyph commented May 23, 2016

I just saw this land in a release. Once upon a time, @fijal told me that __slots__ were actually bad for PyPy. Summoning him to this ticket, I hope: did I even understand that correctly at the time? If so, is it still true? I want to use this because I love the added "no extra attributes" strictness, but I don't want to hurt performance there.

Loading

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

Successfully merging this pull request may close these issues.

None yet

4 participants