Simplify parsing SSSOM w/ curies.chain
#431
Conversation
This provides an alternative to #429 that makes more explicit the chaining operations done on the metadata and prefix maps This is also a good change to carefully document the way that this is handled, since I might not have captured it accurately
|
But tests failing. |
|
@hrshdhgd before I fix the tests, I want to make sure that everyone agrees on the domain logic (i.e., order of priority for metadata and prefix maps) |
|
Will check tomorrow don't merge |
We're not at merge yet, but if you can give feedback on the business logic for merging that would be great |
|
@matentzn all of the unit test issues are now passing (locally), so this is ready for review (though testing is broken because of poetry, yet again) |
There was a problem hiding this comment.
This looks great, made a few questions and comments. My primary concern right now is roundtripping. For example, what happens right now if someone loads a SSSOM mapping file to, say, annotate it with a new piece of metadata, not having included the SSSOM_BUILT_IN_PREFIXES? Will they just be added to the file that is written out?
TLDR: the remain thing I want to feel convinced about is that if a legal SSSOM file is passed to parse_sssom_table, and then written back out, it remains entirely unchanged. Is that, as far as you can see, the case?
@matentzn I added an explicit test to show that outputting a coherent prefix map is working in b73ee19. If someone adds more data to their MSDF manually, they can clean the prefix map automatically with the I am pretty confident that this makes round-tripping even safer than before, since it always tries to backfill the prefix map with the built-in one when loading, so even slightly broken source data files will get fixed up. |
Closes #363 (final nail in the coffin)
This provides an alternative to #429 that makes more explicit the chaining operations done on the metadata and prefix maps. This is also a good change to carefully document the way that this is handled, since I might not have captured it accurately. As it is, The priority order for combining prefix maps are:
metaprefix_map