draft ADR for schema refactor #36
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as off-topic.
This comment was marked as off-topic.
mslarae13
requested review from
cmungall,
aclum,
corilo,
brynnz22,
anastasiyaprymolenna,
lamccue,
turbomam,
JamesTessmer,
mbthornton-lbl,
SamuelPurvine and
shreddd
This comment was marked as resolved.
This comment was marked as resolved.
Clean up template text from ADR.
decisions/0009-schema-refactor.md
Outdated
| ## Decisions Made | ||
|
|
||
| ### Class:PlannedProcess | ||
| * `Class:PlannedProcess` will be created as a root class |
There was a problem hiding this comment.
PlannedProcess is not a root class. It's a subclass of NamedThing.
There was a problem hiding this comment.
I was meaning to say it is a root for other classes. If root exclusively means the 'top most level' what is the right word here?
There was a problem hiding this comment.
@turbomam I'd like to merge this Friday morning. Any suggestions on a more appropriate word?
|
I'm done reviewing this. Thanks for writing it up! |
|
@mslarae13 this is really good. The comments that appear form me above were written on the 3rd, but they were only visible to me until @aclum helped me release them to the public on the the 5th. I apologize for not checking in with you sooner. I think we can consider most of them low priority discussion starters. I have opened a PR on this branch that emphasizes changing the syntax for referring to schema elements by either linking them or putting just the element name in backticks, since backticks mean this is something you would find literally in a code file or on the command line. I also
I made small changes to almost every line, so the GH diff erroneously makes it look like I overwrote the whole thing. Here's what it looks like, with my suggested changes, when rendered from Markdown. If you don't want to accept my PR as is, I can help to add the parts that you would accept in individually. |
|
@turbomam I'll look at your PR from my branch. But can we call this one done (pending a couple comment responses).. merge my changes into your PR, and then I'll review? I'd like to do your updates separately. |
decisions/0009-schema-refactor.md
Outdated
| ## Decisions Made | ||
|
|
||
| ### Class:PlannedProcess | ||
| * `Class:PlannedProcess` will be created as a root class |
There was a problem hiding this comment.
I was meaning to say it is a root for other classes. If root exclusively means the 'top most level' what is the right word here?
There was a problem hiding this comment.
This is good except for the new notation that it introduces. @mslarae13 I assume you made up this new notation because some people have had trouble reading one of the standard notations, so I'm support of trying something new.
Back-ticks are not just for changing the font used for schema elements. Everything that appears between back-ticks must be present in a source files, in its entirety, verbatim.
So Class:Biosample is not appropriate notation because the string "Class:Biosample" does not appear in any file in src/schema. If you think I'm wrong please share a screenshot or link.
If you think the prefixes are important, then at the very least, they should be moved out of the back-ticks, like Class:Biosample.
In the long run, links are much superior to formatted text. If you want to merge this ADR without links, I can help you add them later. But we shouldn't merge the ADR with the new notation style.
|
Thanks for making those changes and responding to all my feedback, @mslarae13. I am done re-reviewing and responding to all the unresolved conversations I was involved in. |
No description provided.