[SYNPY-1679] Add format column to csv data model. #1276
Conversation
…n rules deprecated
andrewelamb
changed the title
thomasyu888
left a comment
thomasyu888
left a comment
There was a problem hiding this comment.
🔥 I took a look at this, and the overall logic looks sound. I'll defer to the team for a final review.
BryanFauble
left a comment
BryanFauble
left a comment
There was a problem hiding this comment.
These look good to me. Thanks for the changes!
| # TODO: set self.format here instead of passing it to get_validation_rule_based_fields | ||
| # https://sagebionetworks.jira.com/browse/SYNPY-1685 | ||
| explicit_format = self.dmge.get_node_format(node_display_name=self.display_name) | ||
| if explicit_format: |
There was a problem hiding this comment.
Do you have tests that cover this part? The tests verify that format is set correctly when columnType is a string type, but there's no test that verifies a ValueError is raised when columnType is NOT a string type
There was a problem hiding this comment.
Nit: I also think this part can be in a separate function to improve testability
| if format_value is None: | ||
| return format_value | ||
| format_string = str(format_value).lower() | ||
| try: |
There was a problem hiding this comment.
I think this try-except block is also not tested in test_get_node_format
There was a problem hiding this comment.
Nit: I am also wondering whether this part could be changed to call check_allowed_values directly, given that its purpose is to verify whether the values are allowed.
|
@linglp Looks like I was a little quick merging this into develop. I'll create a separate PR for your suggestions when I'm back after Thanksgiving. |
SYNPY-1679
Problem:
When creating JSON Schemas, Schematic/Curator extension was relying on the presence of URL and Date validation rules to know when to add the
formatkeyword.Solution:
The
formatcolumn was added to data model CSVs. This allows users to set theformatkeyword explicitly.