FAIR DO MoMEnT simple type system support - #408
Conversation
… keys to string for speedup and avoid ambiguous result of has(this)
🎨 Chromatic Visual Tests✅ No visual changes ReviewChromatic provides automated visual testing and review for component changes. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #408 +/- ##
==========================================
- Coverage 77.69% 76.99% -0.71%
==========================================
Files 50 51 +1
Lines 3520 3569 +49
Branches 1010 1048 +38
==========================================
+ Hits 2735 2748 +13
- Misses 783 819 +36
Partials 2 2
🚀 New features to boost your workflow:
|
Test ResultsCoverage Summary
Coverage & Quality Reports📊 Codecov Report For more details, check the workflow run |
🎨 Chromatic Visual Tests✅ No visual changes ReviewChromatic provides automated visual testing and review for component changes. |
Test ResultsCoverage Summary
Coverage & Quality Reports📊 Codecov Report For more details, check the workflow run |
| console.log("Return from handleMap for PID ", this); | ||
| return handleMap.get(this.toString()); | ||
| } | ||
| else if(this.prefix.toUpperCase().match('^0\\.SIMPLE')){ |
There was a problem hiding this comment.
I don't like this prefix. We do not have any authority over it. Wouldn't it be better to use one of our own prefixes instead? Also, I would prefer if it weren't hardcoded here. Either hard-code it in the GitHubRegistry Util (belongs there structurally) or make it configurable.
There was a problem hiding this comment.
Sure, it might be not a solution for ever, but for the moment it gave me control and readability. Sooner or later I would definitely like to switch to real PIDs (I think using only our prefix but custom suffixes is not an option), but currently, I think this would also be only a temporary solution, e.g., my initial idea was to refer to the file in GitHub via PID redirect. The reason why I hardcoded it here was because of the implementation of isResolvable, where related strings are also hardcoded, but it's also fine to move it.
| } | ||
|
|
||
| export class GitHubRegistryUtil { | ||
| private static readonly GITHUB_TREE_API = 'https://api.github.com/repos/ThomasJejkal/simple-type-registry/git/trees/main?recursive=1'; |
There was a problem hiding this comment.
Wouldn't a map of the prefix, raw base URL, and tree API URL be better. What happens if we want to support two repos (thinking about Andreas' repo for the FDO type system). For now this map could be hardcoded, but in the future, this would be configured/computed. Based on the current DTR infrastructure, I expect that we need to be able to deal with multiple DTRs soon. The prefix of an attribute could be a good indicator of the DTR used and could be used to configure appropriate types.
There was a problem hiding this comment.
This would assume, that we'll multiple GitHub-based DTRs in future, which I currently don't see. And the combination of prefix, raw base URL, and API URL seems to be specific for GitHub.
While supporting different DTRs will probably be required in future, I think, that their implementations might be completely different, API-wise and mapping-wise, and I did not want to modify the pid-component in such a drastic way. And regarding the corellation between prefix and DTR...not sure if this holds. If you check the first conversation above, an option is also to use our prefix and a "normal" PID to refer to the raw file in GitHub. Then, there is no difference anymore.
| } | ||
|
|
||
| /** | ||
| * Resets the initialization state (useful for testing). |
There was a problem hiding this comment.
This isn't a clean reset. The values are still in typeMap and handleMap.
There was a problem hiding this comment.
Agree, and it even might not be needed.
| */ | ||
| isResolvable(): boolean { | ||
| return !unresolvables.has(this) && !this.prefix.toUpperCase().match('^(0$|0\\.|HS_|10320$)'); | ||
| return !unresolvables.has(this.toString()) && !this.prefix.toUpperCase().match('^(0$|0\\.(?!SIMPLE)|HS_|10320$)'); |
There was a problem hiding this comment.
You would not have this problem if you used our prefix.
There was a problem hiding this comment.
Which problem? The only difference would be, that we can omit checking for 0.SIMPLE.
2 participants
This PR implements the support for resolving FAIR DOs created by the current version of FAIR DO MoMEnT, e.g., 21.11152/8a0091ce-792f-4a78-8b6b-e4b58393e809. The reason for the modification is, that such FAIR DOs are not (yet) using DataTypes registered in the official DTR, but custom DataTypes registered in a dedicated GitHub repository.
Changes are the following: