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

Add empty __slots__ to Expr and Node classes. #1274

Merged
merged 7 commits into from Nov 3, 2015

Conversation

@kwmsmith
Copy link
Member

commented Oct 12, 2015

See issue #1268.

Consistently uses __slots__ in class hierarchies, even when empty.

@cpcloud

This comment has been minimized.

Copy link
Member

commented Oct 12, 2015

Hm it'd be great to have a test. Maybe something as simple as

assert Expr.__slots__ == ()
assert Node.__slots__ == ()
@cpcloud

This comment has been minimized.

Copy link
Member

commented Oct 12, 2015

When you're ready to merge please also add a tiny release note blurb to docs/source/whatsnew/0.5.0.txt

@kwmsmith

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2015

I agree a test would be good, but it feels strange to test an internal detail like "does this class have an empty __slots__." It feels like testing the API for a private method. I don't know of any better way to test for this, though, so I'll add the asserts you mention.

@cpcloud

This comment has been minimized.

Copy link
Member

commented Oct 12, 2015

Yes that test is definitely a bit awkward. If there's a way to test it using the user facing API, by all means

@kwmsmith

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2015

When you're ready to merge please also add a tiny release note blurb to docs/source/whatsnew/0.5.0.txt

Should that be 0.8.4.txt? The latest tag I see is 0.8.3.

@cpcloud

This comment has been minimized.

Copy link
Member

commented Oct 12, 2015

Sorry I meant 0.9.0. I've moved the version to a new minor version since we're dropping support for things.

@cpcloud

This comment has been minimized.

Copy link
Member

commented Oct 24, 2015

@kwmsmith merge master back into your branch it should be passing now

cpcloud added a commit that referenced this pull request Nov 3, 2015

Merge pull request #1274 from kwmsmith/bugfix/add-slots
Add empty __slots__ to Expr and Node classes.

@cpcloud cpcloud merged commit b97a21d into blaze:master Nov 3, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.