8000 #86 Repository location stripping by graysonarts · Pull Request #88 · tableau/document-api-python · GitHub
[go: up one dir, main page]

Skip to content

#86 Repository location stripping #88

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 4 commits into from
Oct 4, 2016
Merged

#86 Repository location stripping #88

merged 4 commits into from
Oct 4, 2016

Conversation

graysonarts
Copy link
Contributor

Addresses #86

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.

🚀

@@ -105,10 +105,10 @@ def save_into_archive(xml_tree, filename, new_filename=None):

def _save_file(container_file, xml_tree, new_filename=None):

if container_file is None:
container_file = new_filename
if new_filename is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, thanks!

@@ -213,6 +213,10 @@ def version(self):
def connections(self):
return self._connections

def clear_repository_location(self):
tag = self._datasourceXML.find('./repository-location')
Copy link
Contributor

Choose a reason for hiding this comment

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

Intentional that it's ./ here but .// in the test?

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.

One minor thing after taking a second look

@@ -213,6 +213,10 @@ def version(self):
def connections(self):
return self._connections

def clear_repository_location(self):
tag = self._datasourceXML.find('./repository-location')
self._datasourceXML.remove(tag)
Copy link
Contributor
@t8y8 t8y8 Oct 1, 2016

Choose a reason for hiding this comment

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

This will fail and throw a TypeError if the find returns None -- eg if you call this method twice or it never had a repository-location tag.

Do we just want this to return silently instead of blowing up?

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 dang that was supposed to have an if tag: on it

@t8y8
Copy link
Contributor
t8y8 commented Oct 4, 2016

🚀 🌕 ⏯️ 🌠

@graysonarts graysonarts merged commit 2afd4c2 into tableau:development Oct 4, 2016
@graysonarts graysonarts deleted the repository-location-stripping branch October 4, 2016 18:16
graysonarts pushed a commit that referenced this pull request Oct 7, 2016
* Add the ability to clear repository location on the datasource for retargetting
* fixing a pep8 issue that was missed
* Adding None check for repository-location in case it doesn't exist
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