-
Notifications
You must be signed in to change notification settings - Fork 1
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
#755 FIX unit component duplication BUG #756
Conversation
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 good to me! All I would do is tell sonarcloud that the "Existing Dimension Component" used in the new unit test is not an issue so the code smell disappears.
) -> List[rdf.qb.AttributeComponentSpecification]: | ||
if self._units_component_already_defined: | ||
# Don't define a second units component, the first one will work just fine. | ||
return [] |
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.
Can you add some debug logging here so we can track that we're avoiding serialising a second units component?
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.
self, column_name_uri_safe: str
) -> List[rdf.qb.AttributeComponentSpecification]:
if self._units_component_already_defined:
_logger.debug(
"The component type has already been serialised, %s.",
column_name_uri_safe,
)
# Don't define a second units component, the first one will work just fine.
return []```
is this the right place for it?
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.
that's definitely the right place, but could it say something like Units component already generated. Not generating a second 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.
Yeah cool I'll sort it out
Kudos, SonarCloud Quality Gate passed! |
#755 Bug was noticed during the work on #703.