Update type hints for prefix map merging #454
Conversation
This PR does the following: 1. Creates a literal type for the 3 different prefix map merge modes 2. Updates type hints based on that 3. Renames `prefix_map_mode` to `merge_mode` in the API This doesn't update the name of the flag in the CLI, that can be done in a separate PR since I don't want to worry about changing backwards compatibility here.
| PREFIX_MAP_MODE_MERGED, | ||
| ] | ||
| MergeMode = Literal["metadata_only", "sssom_default_only", "merged"] | ||
| PREFIX_MAP_MODE_METADATA_ONLY: MergeMode = "metadata_only" |
There was a problem hiding this comment.
Never seen this syntax. What does this do, when considering line 40? Looks redundant at first glance (twice the same assignment)
There was a problem hiding this comment.
This makes checking that one of the constants is used properly is checked by type checking. It further builds the string enumeration into the automatic documentation generation
src/sssom/io.py
Outdated
| @@ -59,7 +60,7 @@ def parse_file( | |||
| output: TextIO, | |||
| input_format: Optional[str] = None, | |||
| metadata_path: Optional[str] = None, | |||
| prefix_map_mode: Optional[str] = None, | |||
| merge_mode: Optional[MergeMode] = None, | |||
There was a problem hiding this comment.
All borders have fallen now, but renaming parameters to central methods like this is really annoying for users, but the change makes sense, so lets just break and mend it all now.
There was a problem hiding this comment.
I have a feeling this one is unused - it is only part of a method that should itself be private. In fact, I would rather remove this option completely and have it automatically chain on all instances.
There was a problem hiding this comment.
I reverted this and removed the renames
|
@matentzn actually, I would much rather delete this functionality entirely and only support the |
|
We should be able at CLI time to allow an option to "fail if a prefix is not declared", like a "strict parsing" mode. This prevents cases like completely automated systems that accidentally infer a completely wrong prefix map for a dataset. I'd prefer to continue supporting that functionality, but I would be ok to move MERGED mode as default.. |
|
@matentzn I simplified this PR and took all of the questionable parts out. Now it's just about using type hints better. |
This PR does the following: