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

See issue #1268.

Consistently uses __slots__ in class hierarchies, even when empty.

@cpcloud
Copy link
Member

cpcloud 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
Copy link
Member

cpcloud 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
Copy link
Member Author

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
Copy link
Member

cpcloud 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
Copy link
Member Author

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
Copy link
Member

cpcloud 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
Copy link
Member

cpcloud 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
Add empty __slots__ to Expr and Node classes.
@cpcloud cpcloud merged commit b97a21d into blaze:master Nov 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants