Added the ability to create and modify fields, including aliases 8000 and … #111
Conversation
…calculated fields
8ed1e2e to
8992861
Compare
|
Confirmed we've got the CLA and all is in order! |
There was a problem hiding this comment.
Awesome work! Thank you so much for all of the contributions you've been making while I was away!
I had a few small things that should probably be changed before we merge this in. It's mostly around decomposing some of the functions into smaller chunks (my personal preference)
| """ | ||
| # TODO: A better approach would be to create an empty column and then | ||
| # use the input validation from its "Field"-object-representation to set values. | ||
| # However, creating an empty column causes errors :( |
There was a problem hiding this comment.
What error is it causing?
There was a problem hiding this comment.
The Field objects init() expects some sort of XML as input. Currently, it's not possible to initialize an "empty" Field and fill it with values later on.
tableaudocumentapi/datasource.py
Outdated
| name: Name of the new Field. String. | ||
| datatype: Datatype of the new field. String. | ||
| role: Role of the new field. String. | ||
| type: Type of the new field. String. |
There was a problem hiding this comment.
type is a reserved word. use either field_type (preferred) or type_
tableaudocumentapi/datasource.py
Outdated
| caption = name.replace('[', '').replace(']', '').title() | ||
|
|
||
| # Create the elements | ||
| column = ET.Element('column') |
There was a problem hiding this comment.
Could you extract the xml manipulation into a separate function that is a column tag factory? So it read more like this:
column = xml_factory.create_column(caption, datatype, role, type_, name)
tableaudocumentapi/datasource.py
Outdated
| def calculations(self): | ||
| """ Returns all calculated fields. | ||
| """ | ||
| # TODO: There is a default [Number of Records] calculation. |
There was a problem hiding this comment.
I think as long as we don't allow people to edit [Number of Records] it's okay to return it.
There was a problem hiding this comment.
Great, I've removed the TODO.
| The new calculated field that was created. Field. | ||
| """ | ||
| # Dynamically create the name of the field | ||
| name = '[Calculation_{}]'.format(str(uuid4().int)[:18]) |
There was a problem hiding this comment.
I think we have a function called make_unique or something that does this. I'll take a look to see if I can find it after doing the review.
There was a problem hiding this comment.
There is one in datasource.py (base36encode), but it's used for a different purpose and the value it returns has a different format than the one required here.
Sidenote: you could replace
ALPHABET = ....
with
ALPHABET = string.digits + string.ascii_lowercase
There was a problem hiding this comment.
Yeah, I left it the simple way so the function is easy to grok :)
(And I copy pasted from SO)
tableaudocumentapi/field.py
Outdated
| @@ -24,6 +24,21 @@ | |||
| ] | |||
|
|
|||
|
|
|||
| def argument_is_one_of(*allowed_values): | |||
There was a problem hiding this comment.
I think (again, I need to go check after the review, I might be confusing this with TSC) that we have a module specifically for property decorators. This should probably be in there. If we don't have one, let's start one :)
There was a problem hiding this comment.
It's TSC ;)
I've created a new module and put the decorator functions in there.
tableaudocumentapi/field.py
Outdated
| @@ -43,14 +58,18 @@ def __init__(self, column_xml=None, metadata_xml=None): | |||
| setattr(self, '_{}'.format(attrib), None) | |||
| self._worksheets = set() | |||
|
|
|||
| self._xml = None | |||
There was a problem hiding this comment.
Actually, I have no idea why this is even here. Removed.
| self._xml.set('role', role) | ||
|
|
||
| @property | ||
| def type(self): |
There was a problem hiding this comment.
type is a reserved word. Per pep8, it should be type_, or something more specific like column_type or field_type
There was a problem hiding this comment.
I've removed all 'free floating' variables that are named 'type'.
I didn't change the properties that are named 'type', since they correspond to a XML attribute named 'type' and changing it will throw off anyone using the API. The way it's working, using type() should not be affected by this.
test/test_field_change.py
Outdated
| """ | ||
| return hash(ET.tostring(self.tds._datasourceTree.getroot())) | ||
|
|
||
| def test_change_values(self): |
There was a problem hiding this comment.
There are many similarities between the code in this test and the next one. Is there a way to extract the mostly common code and replace the differences with arguments to the new function?
There was a problem hiding this comment.
Extracted code into a new function.
test/test_field_change.py
Outdated
| self.tds.remove_field(field) | ||
| self.assertNotEqual(state, self.current_hash()) | ||
|
|
||
| def test_change_aliases(self): |
There was a problem hiding this comment.
This one is also very similar to the two above that I previously commented on.
There was a problem hiding this comment.
See above.
|
Hey Russel, thanks for the feedback. I'll try to find some time for a rework either this or next week. Cheers |
|
I've addressed your feedback and updated the PR. Cheers |
tableaudocumentapi/xfile.py
Outdated
| @@ -70,6 +70,16 @@ def get_xml_from_archive(filename): | |||
| return xml_tree | |||
|
|
|||
|
|
|||
| def create_column(caption, datatype, role, field_type, name): | |||
There was a problem hiding this comment.
This looks fine, but xfile is an odd place, I say toss it back in fields.py -- did @RussTheAerialist recommend it go here?
There was a problem hiding this comment.
@RussTheAerialist wanted this as a column tag factory so it would read
column = xml_factory.create_column(caption, datatype, role, type_, name)
So I put it into xfile, since it's the closest thing to a dedicated XML class that I could find in this project. I'll put it back into fields.py
| self._xml.append(aliases) | ||
|
|
||
| # find out if an alias with this key already exists and use it | ||
| existing_alias = [tag for tag in aliases.findall('alias') if tag.get('key') == key] |
There was a problem hiding this comment.
A slightly more idiomatic way to write this is:
existing_alias = next((tag for tag in aliases.findall('alias') if tag.get('key') == key), None)
alias = existing_alias or ET.Element('alias')There was a problem hiding this comment.
That's neat, haven't seen this pattern before :)
|
Hey Tyler, I've moved the function into fields.py as requested. Currently, I think there is no place this function can go without feeling misplaced. Cheers |
|
🇩🇪 🚀 🌔 |
#111) * Added the ability to create and modify fields, including aliases and calculated fields
This PR fixes #103
Adds the ability to:
Includes tests.