[DOP-3921]: Implement concrete Facet type #500
Conversation
| value: str | ||
| sub_facets: Optional[List["Facet"]] = None | ||
|
|
||
| def __lt__(self, other: "Facet") -> bool: |
There was a problem hiding this comment.
These were added for testing purposes so that we can sort the Facet array
…le to have non-facet directives as children of a facet directive
There was a problem hiding this comment.
I moved these back into a separate project directory since they required me to update the test_project.py test() function every time a change was made here that are irrelevant to the test() function.
For example, there was a test to verify that there were no diagnostics, but with these facets, diagnostics are returned for missing facets. Moving this to a separate directory should go through the same code path, but it shouldn't interfere with other tests.
snooty/taxonomy.py
Outdated
| @@ -44,9 +44,10 @@ def get_taxonomy(cls) -> "TaxonomySpec": | |||
| @classmethod | |||
| def validate_key_value_pairs(cls, facet_str_pairs: List[Tuple[str, str]]) -> None: | |||
| taxonomy_ref = asdict(cls.get_taxonomy()) | |||
| facet_pairs = copy.deepcopy(facet_str_pairs) | |||
There was a problem hiding this comment.
Because tuples and strings are both immutable, you don't actually have to do a deepcopy! Just do facet_pairs = facet_str_pairs[:]
There was a problem hiding this comment.
Also, perhaps it would be better for this method to take a Sequence[] than a List[] if we're not mutating it?
snooty/taxonomy.py
Outdated
| try: | ||
| while len(facet_str_pairs) > 0: | ||
| key, value = facet_str_pairs.pop() | ||
| while len(facet_pairs) > 0: |
There was a problem hiding this comment.
This can just be while facet_pairs:
snooty/postprocess.py
Outdated
|
|
||
| if self.parent_stack: | ||
| parent = self.parent_stack[-1][0] | ||
| if isinstance(parent.sub_facets, list): |
There was a problem hiding this comment.
sub_facets is an Optional[list] right? Much better if that's the case to just have
if parent.sub_facets is not None:
parent.sub_facets.append(facet_node)
snooty/parser.py
Outdated
| curr_facet, diagnostics = self.config.load_facets_from_file(facet_path) | ||
|
|
||
| if curr_facet is None: | ||
| logger.warning( |
There was a problem hiding this comment.
I think this actually sounds like a logger.error()
But would it be a diagnostic instead of just a logging message?
There was a problem hiding this comment.
Yes, that's correct! It would be a diagnostic, and it should already be included in the result. I'll remove this log.
snooty/taxonomy.py
Outdated
| taxonomy_ref = asdict(cls.get_taxonomy()) | ||
| facet_pairs = list(facet_str_pairs[:]) |
There was a problem hiding this comment.
Oh, I didn't notice that it's a Sequence. list() already does a copy, so this should just be list(facet_str_pairs)
Ticket
DOP-3921
Notes
This PR implements a new data class to represent the
Facetdata type. It also incorporates facet validation for thefacets.tomlfile.Updates were made in the postprocessor as well to handle the new
Facettype.