[NEAT-235] Excel Importer/Exporter Support Last 🛟 #444
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
doctrino
force-pushed
the
excel-support-last
branch
from
0aa4679
to
3efefbc
Compare
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
doctrino
force-pushed
the
excel-support-last
branch
from
4616cbc
to
8d9e272
Compare
doctrino
commented
May 8, 2024
Comment on lines
-219
to
-226
| elif reference_result: | ||
| sheets = reference_result.sheets | ||
| original_role = reference_result.role | ||
| read_info_by_sheet = reference_result.read_info_by_sheet | ||
| else: | ||
| raise ValueError( | ||
| "No rules were generated. This should have been caught earlier. " f"Bug in {type(self).__name__}." | ||
| ) |
There was a problem hiding this comment.
This case could not happen due to the logic above, so I removed it and thus simplified the logic.
doctrino
commented
May 8, 2024
| @@ -241,25 +257,29 @@ def _create_metadata_sheet_user_rules(self, rules: Rules) -> dict[str, Any]: | |||
| # Excel does not support timezone in datetime strings | |||
| now_iso = datetime.now().replace(tzinfo=None).isoformat() | |||
| is_info = isinstance(rules, InformationRules) | |||
| is_dms = not is_info | |||
There was a problem hiding this comment.
This is dangerous, as it will break if we add asset rules
doctrino
commented
May 8, 2024
| is_dms = not is_info | ||
| is_extension = self.new_model_id is not None | ||
| is_solution = is_extension and self.new_model_id != existing_model_id | ||
| is_dms = isinstance(rules, DMSRules) |
There was a problem hiding this comment.
This whole function needs a rewrite, have captured it in https://cognitedata.atlassian.net/browse/NEAT-240
nikokaoja
approved these changes
May 10, 2024
| sheets: user, last, and reference. The user sheets are used for inputting rules from a user. The last sheets | ||
| are used for the last version of the same model as the user, while the reference sheets are used for | ||
| the model the user is building on. The options are: | ||
| * "user": The rules are written to the user sheets. This is used when you want to modify the rules |
| @@ -170,17 +189,14 @@ def _write_sheets(self, workbook: Workbook, dumped_rules: dict[str, Any], rules: | |||
| for cell in sheet["2"]: | |||
| cell.font = Font(bold=True, size=14) | |||
|
|
|||
| def _write_metadata_sheet(self, workbook: Workbook, metadata: dict[str, Any], is_reference: bool = False) -> None: | |||
| def _write_metadata_sheet(self, workbook: Workbook, metadata: dict[str, Any], sheet_prefix: str = "") -> None: | |||
There was a problem hiding this comment.
this makes us future proof with sheet_prefix , nice !
| metadata["prefix" if is_info else "space"] = self.new_model_id[0] # type: ignore[index] | ||
| metadata["title" if is_info else "externalId"] = self.new_model_id[1] # type: ignore[index] | ||
| metadata["version"] = self.new_model_id[2] # type: ignore[index] | ||
| elif is_solution and self.dump_as == "reference" and rules.reference: | ||
| metadata["prefix" if is_info else "space"] = "YOUR_PREFIX" | ||
| metadata["title" if is_info else "externalId"] = "YOUR_TITLE" |
There was a problem hiding this comment.
title is converted to name
There was a problem hiding this comment.
my thinking is that prefix is turned into both external_id and space, alt would be to remove all illegal characters from title and turn it to external_id
| @@ -292,7 +292,8 @@ def run(self, rules: MultiRuleData) -> FlowMessage: # type: ignore[override, sy | |||
| if role != "input" and role is not None: | |||
| output_role = RoleTypes[role] | |||
|
|
|||
| excel_exporter = exporters.ExcelExporter(styling=styling, output_role=output_role, is_reference=is_reference) | |||
| dump_as = "reference" if is_reference else "user" | |||
There was a problem hiding this comment.
If not mistaken this cannot currently export user-last-ref ?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ended up simplifying the logic in both the Importer and Exporter.