-
Notifications
You must be signed in to change notification settings - Fork 78
Use namedtuple for interval #786
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
Conversation
benjeffery
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this doesn't break anything as the namedtuple supports indexing right? We'd need tests for the new left and right attributes though. Is there much of a perf hit?
Yes
I've changed e.g.
I would hope it would be negligible. But a browse around the internet implies accessing via name is a tad slower (damn), so we could switch to |
|
I don't we need to worry about the perf hit if there is one, it's not going to matter. We do want to explicitly test that we keep the old behaviour, though, and what the new behaviour is. I don't think we should merge this until after 0.3.0, unless there's a good reason. We don't want to diverge to much from the betas and only put vital bugfixes in at this point. |
|
Yep, agreed on the merge freeze and explicit tests. Otherwise this is good. |
45d9f3c to
99b38fd
Compare
|
Explicit test added. Ready to merge after 0.3.0 released. |
|
OK, tagged this with the 0.3.1 milestone so we know when to merge it. |
|
It might be nice (although certainly not vital) to add a Overkill? |
|
I don't see any harm, once it doesn't clash with the namedtuple behaviour. |
benjeffery
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for the extra tests. Happy to have span later as a separate PR.
99b38fd to
0b0e262
Compare
Fixes #784