GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fixes#47 Implement ability to query fields used on a worksheet by graysonarts · Pull Request #54 · tableau/document-api-python

Conversation

graysonarts commented Jul 18, 2016

Copy link

Copy Markdown

Contributor

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Russell Hay added 5 commits

July 18, 2016 15:49


          first stab at an API, not correct in retrospect

c67766e


          Initial revision of api based on API discussions

c30d741


          adding additional testing and enabling lists to be passed in

49077dc


          removing left over stuff from previous attempt


          Some fields are not listed in <column> tags, so we need to construct …

156a3cc

…from <metadata-record>

t8y8 commented Jul 19, 2016

Copy link

Copy Markdown

Contributor

This is a large review so I'll need some time to wrap my head around it (Nice find on the hidden bug in missing fields though!)

Leaving some superficial comments on my first pass

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

t8y8 reviewed

Jul 19, 2016

View reviewed changes

Comment thread

test/assets/datasource_test.twb Outdated

		</window>
		</windows>
		<thumbnails>
		<thumbnail height='104' name='Sheet 1' width='144'>

t8y8 Jul 19, 2016

Copy link

Copy Markdown

Contributor

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.

Clear out the thumbnails, I think they'll just get regenerated as we iterate on these in the future and it keeps the diff a little more managable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Russell Hay added 2 commits

July 19, 2016 13:22


          removing thumbnail

110f34a


          move _column_object_from* to use a named tuple

544cb69

t8y8 reviewed

Jul 19, 2016

View reviewed changes

Comment thread

tableaudocumentapi/multilookup_dict.py Outdated

		return dict.__getitem__(self, key)


		class PredicatedDictionary(object):

t8y8 Jul 19, 2016

Copy link

Copy Markdown

Contributor

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.

I think the code is here is fine, but I'm a little fuzzy on the need for a new type -- would a regular dictionary or data structure do?

It just makes the code read a little more complicated for new contributors. Not a strong objection, mostly curiosity.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

graysonarts Jul 19, 2016

Copy link

Copy Markdown

Contributor Author

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.

bah, this was left over from the previous approach. I'll remove it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Russell Hay added 5 commits

July 19, 2016 13:27


          removing cruft from previous implementation

c174e8c


          renaming retval to something more useful

69eae9a


          cleaning up _is_used_by_worksheet

a62b6b2


          reformatting import statements

8822bc5


          removing PredicateDictionary

96c1e8d

t8y8 reviewed

Jul 19, 2016

View reviewed changes

Comment thread

tableaudocumentapi/datasource.py


		def _is_used_by_worksheet(names, field):
		return any((True for y in names if y in field.worksheets))
		return any((y for y in names if y in field.worksheets))

t8y8 Jul 19, 2016

Copy link

Copy Markdown

Contributor

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.

You can murder even more ()'s -- there's an implicit generator inside the fucntion call to any, so it can just be any(i for i in iterable if condition)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          A workbook doc without any content should still load

3f999e1

t8y8 reviewed

Jul 19, 2016

View reviewed changes

Comment thread

tableaudocumentapi/datasource.py Outdated



		class FieldDictionary(MultiLookupDict):
		def found_in(self, name):

t8y8 Jul 19, 2016

Copy link

Copy Markdown

Contributor

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.

And let the bikeshedding begin!

found_in?
used_in?
used_by?
on_some_level_of_detail_somewhere_in?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

graysonarts Jul 19, 2016

Copy link

Copy Markdown

Contributor Author

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.

yeah found_in seems a bit not right to me. I think used_by hits the right level of obviousness, though maybe we need "used_by_worksheet" to be specific that this only works on worksheets (eventually a worksheet object, but for now, just the name)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

t8y8 Jul 19, 2016

Copy link

Copy Markdown

Contributor

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.

used_by_sheet() feels good to me (Or whatever our docs call a 'sheet' that is only a 'sheet' and not a dashboard or story)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          found_in -> used_by_sheet

f5711cb

t8y8 commented Jul 20, 2016

Copy link

Copy Markdown

Contributor

🚀🚀🚀 (LGTM)

I'm struggling with the basestring thing, I can't put my finger on why (I might prefer explicit py2/py3 checking but I may also just need sleep). I'll submit a PR if the 'better' way ever hits me.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.