-
Notifications
You must be signed in to change notification settings - Fork 11
SPECBASE-42 Define TEMPLATE_ID aligned with HRID #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
gardes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not familiar with all that has been discussed so take my comments with a grain of salt
| 2+^h|*TEMPLATE_ID* | ||
|
|
||
| h|*Description* | ||
| 2+a|Identifier for templates. Lexical form to be determined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to remove "Lexical form to be determined." now that it is determined.
| 2+|`<<_object_id_class,OBJECT_ID>>` | ||
|
|
||
| h|*Invariants* | ||
| 2+a|__Template_ID_validity__: `not matches("^(?:(?<namespace>[a-zA-Z][a-zA-Z0-9_.:/&?=+-]*\.)::) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure the namespace part of the regex is ok?
I am not familiar with all that has been discussed but a couple of observations:
- It seems like it must have a dot at the end of the regex before the :: separator. This seems unintuitive/wrong
- Do we really need the characters .:/&?=+ within the namespace?
- It seems to allow any number of subsequent "." after the first character which seem counter-intuitive.
Maybe something like this regex works better as a replacement:
(?<namespace>(?:(?:[a-zA-Z_][a-zA-Z0-9_-])(?:.[a-zA-Z_][a-zA-Z0-9_-])+]*)+)::
This would enable org.openehr, org.highmed com.my-test or uk.org.my_organisation.
However unlike the above regex, o....-..c&/=+._ would not be supported as an extreme example of a namespace supported by the current namespace suggestion.
Also, should it not be "matches" instead of "not matches"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not sure, i just made the PR so we have something to work on and move the progress.
Thanks for the input. I copied actually your regex from the comment on confluence.
Can you provide me with the full one ?
I did now:
matches("^(?<namespace>(?:(?:[a-zA-Z_][a-zA-Z0-9_-])(?:.[a-zA-Z_][a-zA-Z0-9_-])]*))::)
(?<rm_publisher>[a-zA-Z])-
(?<rm_package>[a-zA-Z][a-zA-Z0-9_])-
(?<rm_class>[a-zA-Z])\.
(?<concept_name>[a-zA-Z][a-zA-Z0-9_-])\.
(?<release_version>v\d+(?:\.\d+){0,2})
(?:-(?<version_status>))?
(?:\.(?<build_count>\d))?$
")
I think your right and it should be matches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think a few required + went missing somewhere and there is an unmatched parentheses after :: and a dot that needed escaping and the version_status regex part that was missing.
Let me try again in total:
matches("/^(?<namespace>(?:[a-zA-Z][a-zA-Z0-9_-]*)(?:\.[a-zA-Z][a-zA-Z0-9_-]*)+)::
(?<rm_publisher>[a-zA-Z]+)-
(?<rm_package>[a-zA-Z][a-zA-Z0-9_]+)-
(?<rm_class>[a-zA-Z]+)\.
(?<concept_name>[a-zA-Z][a-zA-Z0-9_-]+)\.
(?<release_version>v\d+(?:\.\d+){0,2})
(?:-(?<version_status>[a-zA-Z0-9]+))?
(?:\.(?<build_count>\d))?$
")
Please see https://regex101.com/r/vLdk5l/2 to confirm that this is what you want - you can just add more test strings, one per line to see whether they match or not.
|
Everyone should note that if you change a file like |
|
Do you mean that these ascidocs are also generated over MagicDraw not only the UMLs ? This whole construct seems very intransparent to me and most of the spec people :( |
|
The models have been maintained in MagicDraw for about 12 years from memory. All of the files in the directory But I fear there is a lack of knowledge about this pathway, and some people are unwittingly creating PRs to those generated files. All such changes will be lost the next time I or anyone regenerates from MagicDraw. I have posted quite a few times about this, but not sure if people are reading it. There has been a lot of discussion about moving from MagicDraw to e.g. PlantUML, but there's significant work involved. We probably should do it, but we need to plan for a transition period. |
|
Okay, on monday there is the spec meeting, we may discuss it there. |
|
Yes it's via the UML tool. I need about a week to figure out licenses for openEHR and then we can do that. |
|
We should add a text, that states that any (legacy) template_id.value string that matches the regex for HRID is considered an HRID, including the assumptions around semantic versioning. |
|
See https://openehr.atlassian.net/jira/software/c/projects/SPECPR/issues?jql=project%20%3D%20%22SPECPR%22%20ORDER%20BY%20created%20DESC |
Updated the description of TEMPLATE_ID to clarify the use of HRID format and backwards compatibility. For workgroup agreement and discussion see the CR https://openehr.atlassian.net/browse/SPECBASE-42?focusedCommentId=31103
|
I changed the description to reflect the latest consensus. Jelte Zeilstra will suggest a definition for the function (instead of the invariant).
|
Co-authored-by: Jelte Zeilstra <J3173@users.noreply.github.com>
@sebastian-iancu @wolandscat could you help re 3 and 4 please? |
|
how do we mark this in the jira then ? |
|
In jira the CRs need to be per spec (am and base). The PR (problem report) can be singular. |
This is a first PR including the changes, this is to be discussed