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 caption support for datasources #99

Merged
merged 3 commits into from
Oct 18, 2016
Merged

Add caption support for datasources #99

merged 3 commits into from
Oct 18, 2016

Conversation

graysonarts
Copy link
Contributor

Addresses #98

Copy link
Contributor

@t8y8 t8y8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking, I think the test cleanup can definitely wait until a new PR. Do consider the 'removing caption' case though.


@caption.setter
def caption(self, value):
self._datasourceXML.attrib['caption'] = value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We do .set everywhere else (or everywhere in connections)

elif os.path.isfile(path):
os.unlink(path)

def get_temp_file(self, filename):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 3 ways of doing temp file management now.

xfile.temporary_directory has a context manager that creates and deletes a temp dir
bvt.py does a less advanced version of your cleanup methods here
Then these cleanup methods here.

This can be another PR, maybe we want to unify them, at least across the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'll do a pass to create a "temp file wrapper that both library code and tests can use" along with the auto clean up base test class.

def caption(self):
return self._caption

@caption.setter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all datasources guaranteed to have a caption or would you ever want to remove it?

We could apply the None strategy like we do for port

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's more idiomatic to do del ds.caption

@t8y8
Copy link
Contributor

t8y8 commented Oct 18, 2016

🚀

@graysonarts graysonarts merged commit 9e1d875 into tableau:development Oct 18, 2016
@graysonarts graysonarts deleted the 98-add-caption-support branch October 18, 2016 21:24
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