Skip to content

Adding a new feature to find connections and test cleanup#12

Closed
t8y8 wants to merge 1 commit into
tableau:masterfrom
t8y8:get-connections-by-name2
Closed

Adding a new feature to find connections and test cleanup#12
t8y8 wants to merge 1 commit into
tableau:masterfrom
t8y8:get-connections-by-name2

Conversation

@t8y8
Copy link
Copy Markdown
Contributor

@t8y8 t8y8 commented May 20, 2016

  1. Cleaned up some tests to be more consistent
  2. Added many more of them
  3. Cleaner implementation of "find by any attribute"
  4. Left in "find by name" -- but could remove this since it's redundant

… by connection attributes.

Added lots more tests and moved some of that around. Revealed some bugs in my example TDS (It was actually the wrong XML).
self._authentication = connxml.get('authentication')
self._class = connxml.get('class')

def __repr__(self):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why represent this in a "made-up" style instead of simply representing it as it would look in xml?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The repr is mainly for troubleshooting and printing to stdout -- it's common to enclose it in angle brackets and include a memory address -- here I just went with something that was 'not real' enough to notice that it's a debug thing that I'm printing but enough to tell me it's the right object.

It could be:
"<Connection server={} dbname={} class={} /> which is probably usually enough to note if it's the right connection or not.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

feels weird to me to not use the existing representation but I have not worked through all the debug output yet so am not yet "passionate". Note, for xml, the element names start lower case. :) Thanks!!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The repr we get by default is <tableaudocumentapi.connection.Connection object @ abcd1234>

Ha, oh XML.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hahaha. Okay ... then Connection makes sense from the object sense. I guess we should figure out if we want it to look like the xml or the object (which might have more information) and then be clear. Perhaps your initial pass was better to make it clear it is not the raw xml and we could have a different way to dump that. I am cool with that

@t8y8
Copy link
Copy Markdown
Contributor Author

t8y8 commented Jun 18, 2016

Discarding this in favor of a smaller PR with just the test cleanup

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