8000 Fixes #50 - materialize the properties for completion by graysonarts · Pull Request #52 · tableau/document-api-python · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 6 commits into from
Jul 14, 2016
Merged

Fixes #50 - materialize the properties for completion #52

merged 6 commits into from
Jul 14, 2016

Conversation

graysonarts
Copy link
Contributor

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


@property
def is_ordinal(self):
return self._type == 'ordinal'
Copy link
Contributor
@t8y8 t8y8 Jul 12, 2016

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?

Copy link
Contributor Author

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

@t8y8
Copy link
Contributor
t8y8 commented Jul 12, 2016

LGTM

@t8y8
Copy link
Contributor
t8y8 commented Jul 13, 2016

This is just a suggestion, but it reads a little cleaner to me:

_notthere = object()

    def get(self, key, default_value=_notthere):
        try:
            return self[key]
        except AttributeError:
            if default_value is _notthere:
                raise
        return default_value

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

Russell Hay added 4 commits July 13, 2016 09:45
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
@graysonarts
Copy link
Contributor Author
graysonarts commented Jul 13, 2016

@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:
Copy link
Contributor

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

Copy link
Contributor Author

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!

Copy link
Contributor Author

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

@t8y8
Copy link
Contributor
t8y8 commented Jul 13, 2016

Now that's one classy PR

LGTM

@graysonarts graysonarts merged commit f46f3d9 into tableau:development Jul 14, 2016
@graysonarts graysonarts deleted the feature-50-better-field-code-completion branch July 14, 2016 18:12
6172
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0