-
Notifications
You must be signed in to change notification settings - Fork 0
Additional 232 enhancements #98
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
Conversation
lattice/schema.py
Outdated
|
|
||
|
|
||
| # Module functions | ||
| def unname_group(expression: RegularExpressionPattern) -> str: |
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.
Not a great function name, but short.
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.
remove_named_regex_groups?
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.
...or maybe this should be a member function of RegularExpressionPattern called normalize, cleaned, remove_groups?
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.
I can make it a static or class method, then we can make it a member if/when the "base class" is implemented.
| if this < other: | ||
| dependency_graph.add_edge(this, other) | ||
| dependency_graph.remove_nodes_from(list(networkx.isolates(dependency_graph))) | ||
| # dependency_graph.remove_nodes_from(list(networkx.isolates(dependency_graph))) |
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.
Not sure why I was removing unconnected nodes; this was causing the Courier declaration to go missing!
| entry += f"{self._indent}\t{e},\n" | ||
| entry += f"{self._indent}\tUNKNOWN\n{self._indent}{self._closure}" | ||
| if "UNKNOWN" not in self._enumerants: | ||
| entry += f"{self._indent}\tUNKNOWN\n" |
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.
ClimateInformation::DataSourceType already has an UNKNOWN enumerant in the schema. We've been adding one in all the other enums just for the cpp code. So, now we check first.
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.
hmmm...do we remember why? This might be worth looking into more later. We can make an issue for now.
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.
My answer is usually, "Chip".
tanaya-mankad
left a comment
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.
See comments on changes.
| ) | ||
| # else: | ||
| # raise Exception(f"Unrecognized Object Type, \"{object_type}\" in {self.file_path}") | ||
| for group in self.data_groups.values(): |
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.
Separated the extraction of elements into its own loop, in order to have all DataGroup objects available when any element is populated. This allows for an element's type to find its defining DataGroup and see if custom attributes should be applied based on that DataGroup's parents and/or templates.
|
@nealkruis, this is ready for re-review. We should merge this before the recent PR I started with custom-cpp-gen-path. |
lattice/schema.py
Outdated
| # Module functions | ||
|
|
||
|
|
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.
Looks like a remnant?
|
|
||
| self.data_element_name = match.group(1) # TODO: Named groups? | ||
| self.data_element_value = match.group(5) | ||
| self.data_element_name = match.group("DataElementName") |
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.
❤️
lattice/schema.py
Outdated
| self.dictionary = data_group_dictionary | ||
| self.parent_schema = parent_schema | ||
| self.parent_template: DataGroupTemplate | None = self._assign_template( | ||
| self.dictionary.get("Data Group Template", "") |
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.
If a template name doesn't exist in the schema (even ""), we should raise an exception or log an error.
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.
I'm not sure I understand this - self.parent_template is allowed to be None because not every DataGroup has one.
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.
What happens if a user has a Data Group Template called ""? That should raise an error since that Data Group Template doesn't exist.
Or if they user references a Data Group Template name that doesn't exist, I think the current code would just assign it to None and go along its happy way. Right?
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.
Oh, I see - None isn't the right answer. Fixed!
lattice/lattice.py
Outdated
| submodule_urls: list[str] = [] | ||
|
|
||
| config_file = Path(self.root_directory / "cpp" / "config.yaml").resolve() | ||
| config_file = Path(self.root_directory / "cpp" / "0config.yaml").resolve() |
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.
Looks like a typo?
dodo.py
Outdated
| "name": name, | ||
| "task_dep": [f"validate_schemas:{name}", f"generate_json_schemas:{name}", f"validate_example_files:{name}"], | ||
| "file_dep": [schema.file_path for schema in example.schemas] | ||
| "file_dep": [schema.schema.file_path for schema in example.schemas] |
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.
I'm slightly bothered by "schema.schema". Why is it necessary?
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.
Yeah, it's unfortunate. We can do it a different way, but the reason this exists is because of how lattice.py changed its interaction with what's now schema.py. Used to be that the schema class held its own json_schema_path, meta_schema_path, and cpp_path. Those no longer exist, so I encapsulated them with the schema in a dataclass called SchemaSupport.
Lattice.schema is now a list[SchemaSupport] instead of a list[Schema].
It's a hack for sure, but it's also structurally sound!
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.
I see. We need a structure to relate the various paths with the Schema since the Schema class doesn't actually know about these paths. I think we just need better names then.
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.
What about Lattice.schema_supports?
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.
Ooops. I see you already changed it to schema_info. I'm okay with that. But the CI is now angry.
String: Object Type: "Data Type"