fix: refactor primitive union refs and restructure the repo #16
Conversation
satyam-mishra-pce
requested review from
aspiers,
bitbeckers and
s-adamantine
aspiers
left a comment
aspiers
left a comment
There was a problem hiding this comment.
Thanks, I like the tidy-up into directory hierarchies a lot! A couple of comments on the namespacing.
| { | ||
| "lexicon": 1, | ||
| "id": "org.hypercerts.claim", | ||
| "id": "org.hypercerts.claim.claim", |
There was a problem hiding this comment.
This double claim doesn't really make sense. On Telegram it looks like we have at least initial consensus on org.hypercerts.impact.* so in this case it would be org.hypercerts.impact.claim.
There was a problem hiding this comment.
Makes sense. I was unsure of what to keep the name (but it was blocking me with some other development), so I just chose this for a quick resolution.
So, shall I replace all the org.hypercerts.claim.* lexicons with org.hypercerts.impact.*? Or is there some discussion still remaining?
app/certified/defs.json
Outdated
| @@ -0,0 +1,71 @@ | |||
| { | |||
| "lexicon": 1, | |||
| "id": "app.certified.defs", | |||
There was a problem hiding this comment.
Is there a good reason to use app.certified here instead of org.hypercerts? I think we should be consistent and just use org.hypercerts.
The location lexicon is more general than even Hypercerts, so it will go in its own namespace, but I need to discuss with @johnx25bd so in the mean time we were just using app.certified.* for the location lexicon only, but not for the others.
There was a problem hiding this comment.
I didn't bother to change the NSIDs because I wasn't sure if it was a mistake or it had a real reason behind it. Let's update it, once you have the discussion. I will update the defs lexicons and all their references to use "org.hypercerts".
This PR addresses the following changes:
unions only allowrefs that are an object type or can directly be mapped to object types, therefore, I updated the definitions that were not an object.smallImageandlargeImageto accept only image mime types.An unimportant change, but its my OCD that led me to restructure the repository and rename the files. [I can undo if you have some other plans]
I also saw that all the schemas' NSIDs started with "org.hypercerts", except the
location.jsonanddefs.jsonthat used "app.certified.*". I don't know if this was an error, so, regardless I put both the files underapp/certified/.