-
Notifications
You must be signed in to change notification settings - Fork 179
Commenting and Docstring cleanup. A few very small code cleanups #120
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,7 @@ | |
|
|
||
| ######## | ||
| # This is needed in order to determine if something is a string or not. It is necessary because | ||
| # of differences between python2 (basestring) and python3 (str). If python2 support is every | ||
| # of differences between python2 (basestring) and python3 (str). If python2 support is ever | ||
| # dropped, remove this and change the basestring references below to str | ||
| try: | ||
| basestring | ||
|
|
@@ -35,7 +35,7 @@ def _get_metadata_xml_for_field(root_xml, field_name): | |
|
|
||
|
|
||
| def _is_used_by_worksheet(names, field): | ||
| return any((y for y in names if y in field.worksheets)) | ||
| return any(y for y in names if y in field.worksheets) | ||
|
|
||
|
|
||
| class FieldDictionary(MultiLookupDict): | ||
|
|
@@ -87,27 +87,32 @@ def base36encode(number): | |
| return sign + base36 | ||
|
|
||
|
|
||
| def make_unique_name(dbclass): | ||
| def _make_unique_name(dbclass): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't ever be called outside of the class so I made it private. |
||
| rand_part = base36encode(uuid4().int) | ||
| name = dbclass + '.' + rand_part | ||
| return name | ||
|
|
||
|
|
||
| class ConnectionParser(object): | ||
| """Parser for detecting and extracting connections from differing Tableau file formats.""" | ||
|
|
||
| def __init__(self, datasource_xml, version): | ||
| self._dsxml = datasource_xml | ||
| self._dsversion = version | ||
|
|
||
| def _extract_federated_connections(self): | ||
| connections = list(map(Connection, self._dsxml.findall('.//named-connections/named-connection/*'))) | ||
| # 'sqlproxy' connections (Tableau Server Connections) are not embedded into named-connection elements | ||
| # extract them manually for now | ||
| connections.extend(map(Connection, self._dsxml.findall("./connection[@class='sqlproxy']"))) | ||
| return connections | ||
|
|
||
| def _extract_legacy_connection(self): | ||
| return list(map(Connection, self._dsxml.findall('connection'))) | ||
|
|
||
| def get_connections(self): | ||
| """Find and return all connections based on file format version.""" | ||
|
|
||
| if float(self._dsversion) < 10: | ||
| connections = self._extract_legacy_connection() | ||
| else: | ||
|
|
@@ -116,8 +121,8 @@ def get_connections(self): | |
|
|
||
|
|
||
| class Datasource(object): | ||
| """ | ||
| A class for writing datasources to Tableau files. | ||
| """A class representing Tableau Data Sources, embedded in workbook files or | ||
| in TDS files. | ||
|
|
||
| """ | ||
|
|
||
|
|
@@ -145,21 +150,23 @@ def __init__(self, dsxml, filename=None): | |
|
|
||
| @classmethod | ||
| def from_file(cls, filename): | ||
| """Initialize datasource from file (.tds)""" | ||
| """Initialize datasource from file (.tds ot .tdsx)""" | ||
|
|
||
| dsxml = xml_open(filename, cls.__name__.lower()).getroot() | ||
| dsxml = xml_open(filename, 'datasource').getroot() | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realized that this was actually less readable than just making it a literal string, which removed the need for the comment "#Gets the name of the class and normalizes it so it can verify the file it opens" |
||
| return cls(dsxml, filename) | ||
|
|
||
| @classmethod | ||
| def from_connections(cls, caption, connections): | ||
| """Create a new Data Source give a list of Connections.""" | ||
|
|
||
| root = ET.Element('datasource', caption=caption, version='10.0', inline='true') | ||
| outer_connection = ET.SubElement(root, 'connection') | ||
| outer_connection.set('class', 'federated') | ||
| named_conns = ET.SubElement(outer_connection, 'named-connections') | ||
| for conn in connections: | ||
| nc = ET.SubElement(named_conns, | ||
| 'named-connection', | ||
| name=make_unique_name(conn.dbclass), | ||
| name=_make_unique_name(conn.dbclass), | ||
| caption=conn.server) | ||
| nc.append(conn._connectionXML) | ||
| return cls(root) | ||
|
|
@@ -194,16 +201,10 @@ def save_as(self, new_filename): | |
|
|
||
| xfile._save_file(self._filename, self._datasourceTree, new_filename) | ||
|
|
||
| ########### | ||
| # name | ||
| ########### | ||
| @property | ||
| def name(self): | ||
| return self._name | ||
|
|
||
| ########### | ||
| # version | ||
| ########### | ||
| @property | ||
| def version(self): | ||
| return self._version | ||
|
|
@@ -222,9 +223,6 @@ def caption(self): | |
| del self._datasourceXML.attrib['caption'] | ||
| self._caption = '' | ||
|
|
||
| ########### | ||
| # connections | ||
| ########### | ||
| @property | ||
| def connections(self): | ||
| return self._connections | ||
|
|
@@ -234,16 +232,15 @@ def clear_repository_location(self): | |
| if tag is not None: | ||
| self._datasourceXML.remove(tag) | ||
|
|
||
| ########### | ||
| # fields | ||
| ########### | ||
| @property | ||
| def fields(self): | ||
| if not self._fields: | ||
| self._fields = self._get_all_fields() | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these added more clutter than they helped, and weren't consistently applied. I left the section headers in place (Public API, Private API)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But then I realized I left several in. I'll remove them |
||
| return self._fields | ||
|
|
||
| def _get_all_fields(self): | ||
| # Some columns are represented by `column` tags and others as `metadata-record` tags | ||
| # Find them all and chain them into one dictionary | ||
| column_field_objects = self._get_column_objects() | ||
| existing_column_fields = [x.id for x in column_field_objects] | ||
| metadata_only_field_objects = (x for x in self._get_metadata_objects() if x.id not in existing_column_fields) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,25 +11,23 @@ | |
|
|
||
|
|
||
| class Workbook(object): | ||
| """ | ||
| A class for writing Tableau workbook files. | ||
|
|
||
| """ | ||
| """A class for writing Tableau workbook files.""" | ||
|
|
||
| ########################################################################### | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so are we leaving this comment in to denote the public API? fine if we do just checking
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left the public and private api bits in, I personally am not a fan, so I'm happy to remove them. But I was deferring to your original commit 🍭 |
||
| # | ||
| # Public API. | ||
| # | ||
| ########################################################################### | ||
| def __init__(self, filename): | ||
| """ | ||
| Constructor. | ||
| """Open the workbook at `filename`. This will handle packaged and unpacked | ||
| workbook files automatically. This will also parse Data Sources and Worksheets | ||
| for access. | ||
|
|
||
| """ | ||
|
|
||
| self._filename = filename | ||
|
|
||
| self._workbookTree = xml_open(self._filename, self.__class__.__name__.lower()) | ||
| self._workbookTree = xml_open(self._filename, 'workbook') | ||
|
|
||
| self._workbookRoot = self._workbookTree.getroot() | ||
| # prepare our datasource objects | ||
|
|
@@ -42,23 +40,14 @@ def __init__(self, filename): | |
| self._workbookRoot, self._datasource_index | ||
| ) | ||
|
|
||
| ########### | ||
| # datasources | ||
| ########### | ||
| @property | ||
| def datasources(self): | ||
| return self._datasources | ||
|
|
||
| ########### | ||
| # worksheets | ||
| ########### | ||
| @property | ||
| def worksheets(self): | ||
| return self._worksheets | ||
|
|
||
| ########### | ||
| # filename | ||
| ########### | ||
| @property | ||
| def filename(self): | ||
| return self._filename | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,19 +22,23 @@ class TableauInvalidFileException(Exception): | |
|
|
||
|
|
||
| def xml_open(filename, expected_root=None): | ||
| """Opens the provided 'filename'. Handles detecting if the file is an archive, | ||
| detecting the document version, and validating the root tag.""" | ||
|
|
||
| # Is the file a zip (twbx or tdsx) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's be consistent with what is in datasource.py and prepend each extension with a ".". so make this ".twbx or .tdsx" please |
||
| if zipfile.is_zipfile(filename): | ||
| tree = get_xml_from_archive(filename) | ||
| else: | ||
| tree = ET.parse(filename) | ||
|
|
||
| # Is the file a supported version | ||
| tree_root = tree.getroot() | ||
|
|
||
| file_version = Version(tree_root.attrib.get('version', '0.0')) | ||
|
|
||
| if file_version < MIN_SUPPORTED_VERSION: | ||
| raise TableauVersionNotSupportedException(file_version) | ||
|
|
||
| # Does the root tag match the object type (workbook or data source) | ||
| if expected_root and (expected_root != tree_root.tag): | ||
| raise TableauInvalidFileException( | ||
| "'{}'' is not a valid '{}' file".format(filename, expected_root)) | ||
|
|
@@ -79,6 +83,10 @@ def get_xml_from_archive(filename): | |
|
|
||
|
|
||
| def build_archive_file(archive_contents, zip_file): | ||
| """Build a Tableau-compatible archive file.""" | ||
|
|
||
| # This is tested against Desktop and Server, and reverse engineered by lots | ||
| # of trial and error. Do not change this logic. | ||
| for root_dir, _, files in os.walk(archive_contents): | ||
| relative_dir = os.path.relpath(root_dir, archive_contents) | ||
| for f in files: | ||
|
|
||
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.
is there a list of all available options somewhere? if so would be nice to point ppl to that
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.
The dbclass.py file lists my best guess at a comprehensive list that I extracted from our CPP code :), I can point them to that