Meta tab #144
Meta tab #144
Conversation
Was just bugging me and I didn't want to try to reconcile other changes I've made locally
| "--metatab-schema", | ||
| help="The jsonschema of the metadata tab") | ||
| parser_unflatten.add_argument( | ||
| "--metatab-only", |
There was a problem hiding this comment.
Should this have action='store_true' ?
| sheet_names = sorted([fname[:-4] for fname in sheet_file_names if fname.endswith('.csv')]) | ||
| if self.include_sheets: | ||
| for sheet in list(sheet_names): | ||
| if sheet not in self.include_sheets: |
There was a problem hiding this comment.
Should this be case insensitive? The metatab spec isn't consistent with case for "Meta"
There was a problem hiding this comment.
Happy to leave this as case sensitive for now, but we might need to come back to this in future, as I think Excel users generally expect the MS pattern of case insensitive but preserving (e.g. I can't create two tabs with the same name in different cases).
flattentool/input.py
Outdated
| for sheet in list(self.sheet_names_map): | ||
| if sheet not in self.include_sheets: | ||
| self.sheet_names_map.pop(sheet) | ||
| for sheet in list(self.exclude_sheets) or []: |
There was a problem hiding this comment.
Why list( on line 448 and 451?
There was a problem hiding this comment.
list is needed on 448 as you are not able to pop things from a map while iterating over it (in python3 as it creates a generator).
It is not needed on 451 so will remove it there.
flattentool/input.py
Outdated
| cells[header] = Cell(line[header], (sheet_name, _get_column_letter(k+1), j+2, actual_headings[k])) | ||
| headings = actual_headings[k] if actual_headings else header | ||
| if self.vertical_orientation: | ||
| cells[header] = Cell(line[header], (sheet_name, str(k+1), j+2, headings)) |
There was a problem hiding this comment.
Shouldn't this be (sheet_name, _get_column_letter(j+2), k+1, headings)?
There was a problem hiding this comment.
No, because this would lead to broken "Rows" output in Cove. We think there's also problems with this/the current approach, but there's a new issue for this #153
flattentool/input.py
Outdated
| @@ -246,10 +260,11 @@ def do_unflatten(self): | |||
| root_id_or_none = line.get(self.root_id) if self.root_id else None | |||
| cells = OrderedDict() | |||
| for k, header in enumerate(line): | |||
| if actual_headings: | |||
| cells[header] = Cell(line[header], (sheet_name, _get_column_letter(k+1), j+2, actual_headings[k])) | |||
| headings = actual_headings[k] if actual_headings else header | |||
There was a problem hiding this comment.
Should be heading not headings.
Fix some naming and add comment from review. Also remove an not needed list.
|
@kindly Thanks for the changes to this. I'm happy to see this merge now. I can't actually approve it, because I created the pull request! |
Allow metadata that will appear at the top level, e.g. version number, to be specified in an additional spreadsheet tab.