Add suport for XLSX imports #898
Conversation
|
|
||
| class Importer(): | ||
|
|
||
| def __init__(self, project): | ||
| def __init__(self, project=None, delimiter=None, quotechar=None): |
There was a problem hiding this comment.
You could write this method as:
def __init__(self, project=None, delimiter=',' quotechar='"'):
self.project = project
self._schema_attrs = {}
self.delimiter = delimiter
self.quotechar = quotecharMakes it a bit easier to read.
Also is project actually optional here? If not we should not define it as an optional keyword argument.
| row, content_types, attributes, | ||
| attr_map, tenure_type | ||
| ) | ||
| except Exception as e: |
There was a problem hiding this comment.
What exceptions do you expect here. We should only catch those instead of Exception, which covers all exceptions that could possibly happen. That means if something is unexpectedly going wrong in this bloc, we will have a very hard time debugging the problem.
There was a problem hiding this comment.
Refactored to catch ValidationErrors only.
| # handle select_multiple fields | ||
| if (attribute.attr_type.name == 'select_multiple'): | ||
| val = [v.strip() for v in val.split(',')] | ||
| content_types[content_type][ |
There was a problem hiding this comment.
This fits into the line's character limit. Maybe we should make this one line for better readability.
| content_types[content_type][ | ||
| 'attributes'][ | ||
| attribute.name] = val | ||
| party = Party.objects.create( |
There was a problem hiding this comment.
The next few commands statements all fit into one line. Having these in one line will make it easier to read.
| 'tenure_type.label'] | ||
| final.drop(drop_cols, inplace=True, axis=1) | ||
| return final.to_csv(index=False, index_label=False) | ||
| except Exception: |
There was a problem hiding this comment.
Again, we should catch generic exceptions, only those that we actually expect in the block.
There was a problem hiding this comment.
Refactored to catch KeyError's only here.
| url = '' | ||
| while not url: | ||
| temp_url = upload_to + '/' + random_id() + ext | ||
| try: |
There was a problem hiding this comment.
Why don't you use if not Resource.objects.filter(file__contains=temp_url).exists() here? Saves you two lines and is easier to read...
bjohare
force-pushed
the
enhancement/#832
branch
3 times, most recently
from
2e1e61f
to
e2c61f4
Compare
bjohare
force-pushed
the
enhancement/#832
branch
2 times, most recently
from
2f7d4e0
to
6c012c7
Compare
oliverroick
force-pushed
the
enhancement/#832
branch
from
6c012c7
to
8efb65a
Compare
* Add XLSX imports * Add import of Locations or Parties separately * Add selection of location_type field in import * Add selection of party_type field in import * Add suport for 1-M relations in import
oliverroick
force-pushed
the
enhancement/#832
branch
from
8efb65a
to
9f30ab0
Compare
Proposed changes in this pull request
Fix #832 Data Import: Add support for XLSX imports
When should this PR be merged
Can be merged immediately.
Risks
Low risk
Follow up actions
Adds dependency on
pandas 0.19.0, re-provisioning required