-
Notifications
You must be signed in to change notification settings - Fork 3
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
BDRSPS-1028 Created specific relatedSiteID custom check and applied to site data template v2 #348
Conversation
…lied that to the validation of relatedSiteID on the site template.
…ied it to the validation of survey site data 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.
Looks good, 2 comments and 1 FYI
""" | ||
check = ( | ||
row["relatedSiteID"] in self.site_ids | ||
if row["relatedSiteID"] and row["relationshipToRelatedSite"].lower().replace(" ", "") == "partof" |
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.
rather than
row["relationshipToRelatedSite"].lower().replace(" ", "") == "partof"
you could use
vocabs.relationship_to_related_site.PART_OF.match(row["relationshipToRelatedSite"])
in a similar fashion to here so we don't have another implmentation of matching a value to the vocab
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.
intentionally went this way since it is related to validation and not to mapping, i.e. in the same vane we duplicate enums for fields that map to restricted vocabs in the schema. I am concerned with a bad code smell of adding the vocab dependency to a frictionless plugin module. It may not create a complete circular import but it would be one step away. The example provided was for a mapping function, contained within the module. I could move this custom check into the same module as the site data mapper to completely remove that concern for me but it might make it less readable for others.
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.
intentionally went this way since it is related to validation and not to mapping
ok that makes sense 👍
The check looks at the
relationshipToRelatedSite
and if found to be "partOf" then therelatedSiteID
must be declared as asiteID
in the same template.