GeezerVison

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

GeezerVison is showing github.com — Issue #96: Add port support by t8y8 · Pull Request #97 · tableau/document-api-python

Conversation

t8y8 commented Oct 17, 2016 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

Addresses #96

Adding support for reading the port attribute from connections that have one.

We keep None in the model for cases where there is no port, but that won't get written to the XML.

Setting it to any other value will be persisted in the XML

This is a minimal change to resolve the issue -- I realize that we could do some cleanup on class construction like in the client-python-lib, but I don't think it's necessary in this PR

Sorry, something went wrong.

Uh oh!

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

All reactions


          Issue #96: Add port support

6c988b1

t8y8 commented Oct 17, 2016

Copy link

Copy Markdown

Contributor Author

/cc @RussTheAerialist @geordielad

All reactions

Sorry, something went wrong.

Uh oh!

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

graysonarts suggested changes

Oct 17, 2016

View reviewed changes

Comment thread

tableaudocumentapi/connection.py

		self._port = value
		# If port is None we don't actually want to update the XML
		if value is not None:
		self._connectionXML.set('port', value)

graysonarts Oct 17, 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.

How do you remove the port if it exists and you want to remove it?

Sorry, something went wrong.

Uh oh!

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

All reactions

t8y8 Oct 17, 2016 •

edited

Loading

Uh oh!

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

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.

Hmm. I can invert the logic so that it's:

    @port.setter
    def port(self, value):
        self._port = value
        # If port is None we remove the element and don't write it to XML
        if value is None:
            try:
                self._connectionXML.attrib.pop('port')
            except KeyError:
                pass
        else:
            self._connectionXML.set('port', value)

EDIT: This is an implementation that passes tests

Sorry, something went wrong.

Uh oh!

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

All reactions

graysonarts Oct 17, 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.

That would be good, though I prefer this semantic for deleting rather than poping

del self._connectionXML.attrib['port']

Sorry, something went wrong.

Uh oh!

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

All reactions

t8y8 Oct 17, 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.

Done -- one SO post said del doesn't work, but others said it's fine and it works for me too

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

tableaudocumentapi/connection.py Outdated

		self._username = connxml.get('username')
		self._authentication = connxml.get('authentication')
		self._class = connxml.get('class')
		self.port = connxml.get('port', None)

graysonarts Oct 17, 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.

should this be self._port?

Sorry, something went wrong.

Uh oh!

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

t8y8 reacted with thumbs up emoji

All reactions

👍 1 reaction

t8y8 Oct 17, 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.

I think originally I didn't so that validation logic would get triggered, but with my new "Don't write it" approach I think I can do just _port now.

Sorry, something went wrong.

Uh oh!

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

All reactions


          Respond to CR feedback and fix tests with side effects

c6b392d

graysonarts approved these changes

Oct 17, 2016

View reviewed changes

graysonarts left a comment

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.

🚀

Sorry, something went wrong.

Uh oh!

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

All reactions


          Use del instead of pop

3a38058

t8y8 merged commit 1aa4fc0 into tableau:development

Oct 17, 2016

t8y8 deleted the 96-port-support branch

October 17, 2016 17:50

t8y8 mentioned this pull request

Oct 17, 2016

Add port attribute to Connection object #96

Closed

Reviewers

1 more reviewer

graysonarts

graysonarts approved these changes

Reviewers whose approvals may not affect merge requirements

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.

GeezerVison

Conversation

t8y8 commented Oct 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t8y8 commented Oct 17, 2016

Uh oh!

graysonarts Oct 17, 2016

Choose a reason for hiding this comment

Uh oh!

t8y8 Oct 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graysonarts Oct 17, 2016

Choose a reason for hiding this comment

Uh oh!

t8y8 Oct 17, 2016

Choose a reason for hiding this comment

Uh oh!

graysonarts Oct 17, 2016

Choose a reason for hiding this comment

Uh oh!

t8y8 Oct 17, 2016

Choose a reason for hiding this comment

Uh oh!

graysonarts left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

t8y8 commented Oct 17, 2016 •

edited

Loading

Uh oh!

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

t8y8 Oct 17, 2016 •

edited

Loading

Uh oh!

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