-
Notifications
You must be signed in to change notification settings - Fork 182
Fixes #50 - materialize the properties for completion #52
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
Fixes #50 - materialize the properties for completion #52
Conversation
|
||
@property | ||
def is_ordinal(self): | ||
return self._type == 'ordinal' |
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.
Not sure I know which types are 'ordinal' vs 'nominal' -- can you add a docstring to these three?
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.
sure, I'll have to determine how to word it to make it obvious. The best example I can think of is a date series, they define an order, but I'll dig through docs to see how it's explained
LGTM |
This is just a suggestion, but it reads a little cleaner to me:
And it removes the complaint from the back of my head about using an exception as a sentinel. Tests pass -- but we might want a test to cover exactly what this is trying to accomplish. Now we get a KeyError when you pass an invalid key into the dict |
This also fixes a bug where captions and aliases weren't being populated into the multidict correctly due to insufficient testing on my part originally
@t8y8 - That's much cleaner, I like that better because it makes it impossible to pass in that value. I'll update with this change and some additional tests. Though it might not happen today due to off-site. |
except AttributeError: | ||
if default_value != AttributeError: | ||
except KeyError: | ||
if default_value != _no_default_value: |
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.
Since we're checking for that object you can use "is not" for identity checking, and then it's impossible for any overridden eq to trick it
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.
oh good point, that's what I get for rushing!
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.
Also, boo to me for breaking py3. I've now added running the tests under py3 to my pre-commit hook
Now that's one classy PR LGTM |
This also fixes a bug where captions and aliases weren't being populated
into the multidict correctly due to insufficient testing on my part
originally