Conversation
Signed-off-by: Emmanuel Mireku Omari <omariemmanuel91@gmail.com>
|
One thing that would be helpful for the code review would be a video recording of you using these to show that they work. Also, the PR description should include a test plan (e.g. how to get the snippets properly installed, what someone should to to verify that they work, etc.) In general, developers should always make it as easy as possible for reviewers to do the review job. Assume that a reviewer has a lot of other things to do (almost certainly true) and may not have enough context know what to look for (likely true). The more you can do to make the reviewer's job easy/ efficient, the faster the review will be completed. |
There was a problem hiding this comment.
Some things to address:
- $0 is the final tabstop. Always insert $0 where you want the cursor to end up. Otherwise it will be placed at the end of the snippet content. It is better to be explicit about this. There are many cases where we want the cursor to be somewhere other than after all the snippet content (e.g. any container like DataModel, CDM, LDM, PDM, ...)
- Make sure that tab stop indentation is appropriate.
- Make sure to associate these snippets with just the .uddl, .face, .privacy file extensions (See https://code.visualstudio.com/docs/languages/identifiers)
It is possible to 'daisy chain' snippets together. What I mean is this: Each tab stop can trigger another snippet by simply making the snippet name one of the value choices at that point. If so, when a user picks the snippet name and hits CTRL-Space, it will automatically insert the content of that snippet so the user can keep going. The user should have the choice of picking nothing if it is possible to stop data entry at that point.
See examples here, here or here.
Note that the snippet names are different from keyword names to make sure that you can insert just the keyword if you want to. This is particularly valuable at the last tab stop but can be used anywhere. Note also that using this approach at the $0 tabstop is easy because you don't need to come back the original snippet. If you do need to come back to the original snippet it gets more complicated.
Optional content should also be addressed in the snippets. For example, a ConceptualEntity has the option of specializing another ConceptualEntity. The : should only be there if the ConceptualEntity specializes - once the user is finished with the snippet the ConceptualEntity should be syntactically correct both when it specializes and when it doesn't.
Optional elements can be addressed in several ways:
- Transformation: See examples here
- Subchoices: See links above.
Also - this should become part of a Langium/JS VS Code extension we already have once it lands. Consider looking at Langium (https://langium.org/docs/). A logical next step would be to add scoping to the VS Code support. @papiasharmin has been working on some JS/TS and is somewhat familiar with Langium as a result. She might have some perspective.
| "prefix": "ccquery", | ||
| "body": [ | ||
| "ccquery ${1:name} '${2:description}'{," | ||
| ,"isUnion", |
There was a problem hiding this comment.
'isUnion' is an option. It would be nice if this displays as an option rather than just getting inserted. If the other option is a single space (" ") or comment ("/..../"), it should create a drop down choice that has no semantic consequence. Or just making it a tab stop with a default value of 'isUnion' will give the user the option of deleting the value before moving on. The goal is to make sure the user knows this is an option and then picks the value desired. Otherwise, users could tab past this without realizing its impact.
| "prefix": "pcquery", | ||
| "body": [ | ||
| "pquery ${1:name} '${2:description}' -> ${3:LogicalQuery}{" | ||
| ,"isUnion" |
There was a problem hiding this comment.
See prior comment about isUnion on ConceptualCompositeQuery.
| "prefix": "lcquery", | ||
| "body": [ | ||
| "lquery ${2:name} '$3{description}' -> ${4:ConceptualCompositeQuery}{" | ||
| ,"isUnion" |
There was a problem hiding this comment.
See prior comment about isUnion on ConceptualCompositeQuery.
| "prefix": "lquery", | ||
| "body": [ | ||
| "lquery ${2:name} '$3{description}' -> ${4:ConceptualQuery}{" | ||
| ,"spec: '${5specification}'" |
There was a problem hiding this comment.
5specification -> 5:specification
| "prefix": "dm", | ||
| "body": [ | ||
| "dm: ${1:name} '${2:description}' {", | ||
| "${3:}", |
There was a problem hiding this comment.
Make sure to do proper indentation. For example, line 6 should be indented 1 tab (2 spaces) compared to line 5 above.
| "prefix": "cdm", | ||
| "body": [ | ||
| "cdm ${1:name} '${2:description}' {", | ||
| "\t$3", |
There was a problem hiding this comment.
Here you indent correctly.
| "ConceptualComposition": { | ||
| "prefix": "composition", | ||
| "body": [ | ||
| "${1:type} ${2:rolename}[ ${3:lowerBound} : ${4:upperBound}]","'${5:description}' : ${ConceptualCharacteristic};\n" |
There was a problem hiding this comment.
${ConceptualCharacteristic} -> ${6:ConceptualCharacteristic}
| "prefix": "dm", | ||
| "body": [ | ||
| "dm: ${1:name} '${2:description}' {", | ||
| "${3:}", |
There was a problem hiding this comment.
kindly correct me, if I am wrong, don't we need to add the attributes conceptualDataModel, logicalDataModel and PlatfromDataModel inside ${3:}?
There was a problem hiding this comment.
It will be added as a list with the option to select one
| "ConceptualQueryComposition":{ | ||
| "prefix": "cqcomp", | ||
| "body": [ | ||
| "${1:ConceptualView} ${2:rolename};\n" |
There was a problem hiding this comment.
why ${1:ConceptualView} instead of ${1:type}
There was a problem hiding this comment.
ConceptualView to alert the user that it takes a ConceptualView type
There was a problem hiding this comment.
Have you tested these with a version of VS code that has the UDDL / FACE VS Code extension (.vsix) installed? (Available here ). If not, do so - it may be that the Java content assist will kick in and provide the list of ConceptualViews available to cross reference.
| "};\n" | ||
| ], | ||
| "description": "A conceptual Participant is the mechanism that allows a conceptual Association to be constructed between two or more conceptual Entities." | ||
| }, |
There was a problem hiding this comment.
ConceptualParticipantPathNode and ConceptualCharacteristicPathNode are not declared.
There was a problem hiding this comment.
There will be some elements where there is no advantage to having a snippet. These two are examples of that. The syntax for these is just a single character followed by a cross reference. Snippets have value when there are multiple fields to fill in.
| ], | ||
| "description": "A StandardMeasurementSystem is used to represent an open, referenced measurement system without requiring the detailed modeling of the measurement system. The reference should be unambiguous and allows for full comprehension of the underlying measurement system." | ||
| }, | ||
| "landmark":{ |
| ,"}\n" | ||
| ], | ||
| "description": " A MeasurementSystemAxis establishes additional properties for a CoordinateSystemAxis including units and value types. " | ||
| }, |
There was a problem hiding this comment.
is LogicalReferencePoint not declared intentionally?
There was a problem hiding this comment.
no!!
missed it.
It will be added
| ,"};\n" | ||
| ], | ||
| "description": " A MeasurementAxis optionally establishes constraints for a MeasurementSystemAxis and may optionally override its default units and value types. " | ||
| }, |
There was a problem hiding this comment.
is LogicalMeasurementAttribute not declared intentionally?
There was a problem hiding this comment.
same as LogicalReferencePoint
| "ldouble ${1:name} '${2:description}' -> ${3:LogicalAbstractMeasurement};\n" | ||
| ], | ||
| "description": " A LongDouble is a real data type that represents an IEEE extended double precision floating- point number (having a signed fraction of at least 64 bits and an exponent of at least 15 bits) " | ||
| }, |
There was a problem hiding this comment.
same as LogicalReferencePoint
| "prefix": "cassoc", | ||
| "scope": "com.epistimis.uddl.Uddl.ConceptualBasisEntity", | ||
| "body": [ | ||
| "cassoc ${1:name} '${2:description}' : ${3:ConceptualEntity} {", |
There was a problem hiding this comment.
It seems like we are mentioning all key of each property, like name, description, lowerbound, upperbound etc, then why it is ${3:ConceptualEntity} and not ${3:specializes}.
There was a problem hiding this comment.
same as ConceptualView
| "${1:type} ${2:rolename}[${3:lowerBound} : ${4:upperBound}]", | ||
| "'${5:description}' : ${6:ConceptualCharacteristic}{", | ||
| "src: [${7:sourceLowerBound} : ${8:sourceUpperBound}] ", | ||
| "path: ${8:ConceptualPathNode}", |
There was a problem hiding this comment.
path: ${8:ConceptualPathNode}" -> path: ${9:ConceptualPathNode}",
| "description": "A CoordinateSystem is a system which uses one or more coordinates to uniquely determine the position of a point in an N-dimensional space. The coordinate system is comprised of multiple CoordinateSystemAxis which completely span the space. Coordinates are quantified relative to the CoordinateSystemAxis. It is not required that the dimensions be ordered or continuous." | ||
| }, | ||
| "LogicalCoordinateSystemAxis":{ | ||
| "prefix": "coordAxis", |
There was a problem hiding this comment.
why prefix is "coordAxis" not "csa"
| "LogicalValueTypeUnit":{ | ||
| "prefix": "vtu", | ||
| "body": [ | ||
| "vtu ${1:valueType} ${2:unit}{" |
There was a problem hiding this comment.
name and description attribute is missing.
| ,"'${4:description}';\n" | ||
| ], | ||
| "description": " A FixedLengthStringConstraint specifies a defined set of meaningful values for a String of a specific fixed length. The “length” attribute defines the fixed length, an integer value greater than 0." | ||
| }, |
There was a problem hiding this comment.
LogicalEnumerationConstraint is not defined.
| "LogicalQuery":{ | ||
| "prefix": "lquery", | ||
| "body": [ | ||
| "lquery ${2:name} '$3{description}' -> ${4:ConceptualQuery}{" |
There was a problem hiding this comment.
| "PlatformCharArray":{ | ||
| "prefix": "pcarray", | ||
| "body": [ | ||
| "char[${4:maxLength}] ${1:name} '${2:description}' -> ${3:LogicalAbstractMeasurement};\n" |
| "float ${1:name} '${2:description}' -> ${3:LogicalAbstractMeasurement};\n" | ||
| ], | ||
| "description": "A Float is a real data type that represents an IEEE single precision floating-point number. " | ||
| }, |
| "PlatformStructMember":{ | ||
| "prefix": "pstructmember", | ||
| "body": [ | ||
| " ${1:PlatformDataType} '${2:rolename}'(${4:precision}) -> ${3:LogicalMeasurementAttribute};\n" |
There was a problem hiding this comment.
It should be PlatfomDataType
| "prefix": "pquery", | ||
| "body": [ | ||
| "pquery ${1:name} '${2:description}' -> ${3:LogicalQuery}{" | ||
| ,"spec: '${5:specification}'" |
There was a problem hiding this comment.
'${5:specification}' -> '${4:specification}'
| "ConceptualDomain": { | ||
| "prefix": "domain", | ||
| "body": [ | ||
| "domain ${1:name} '${2:description}';\n", |
There was a problem hiding this comment.
Be consistent about spacing: Compare line 40 to line 47. The ; at the end of line 40 has a preceding
| "};\n" | ||
| ], | ||
| "description": "A conceptual Participant is the mechanism that allows a conceptual Association to be constructed between two or more conceptual Entities." | ||
| }, |
There was a problem hiding this comment.
There will be some elements where there is no advantage to having a snippet. These two are examples of that. The syntax for these is just a single character followed by a cross reference. Snippets have value when there are multiple fields to fill in.
| "ConceptualQueryComposition":{ | ||
| "prefix": "cqcomp", | ||
| "body": [ | ||
| "${1:ConceptualView} ${2:rolename};\n" |
There was a problem hiding this comment.
Have you tested these with a version of VS code that has the UDDL / FACE VS Code extension (.vsix) installed? (Available here ). If not, do so - it may be that the Java content assist will kick in and provide the list of ConceptualViews available to cross reference.
3 participants
No description provided.