initial formatter schema #12

maytheu · 2023-09-11T15:15:12Z

Adding and updating uddl formatter

steve-hickman-epistimis · 2023-09-11T17:52:31Z

It looks like you headed in the right direction

maytheu · 2023-09-12T20:30:09Z

I want to test the formatConceptualEntity() but I don't know where I'll call it

steve-hickman-epistimis · 2023-09-12T22:09:41Z

I want to test the formatConceptualEntity() but I don't know where I'll call it

The protected format(node: AstNode): void method is the only entrypoint for this class. Everything you create must be called directly or indirectly from there. Look at the example here.

Here's what to do: Modify the protected format(node: AstNode): void method to call the format methods for anything that can appear at the top level. Then, inside of those individual format methods, call the format methods for each of the elements that they can hold. And so on.

This is effectively what happens in UddlFormatter.xtend. The top level format method call there is to def dispatch void format(DataModel dataModel, extension IFormattableDocument document). You can see how it calls the format methods of the things it can contain - and they in turn call the format methods of what they can contain. And so on.

What does this mean for testing? It means you never test just one format method by changing what you call - because you have no control of how the format methods get called. You test format methods by what you pass in to be formatted. So to test the formatConceptualEntity() method you need to: A) make sure it is called appropriately as described above; 2) Pass in a string that contains one or more ConceptualEntities (along with whatever else is necessary to create a valid UDDL document).

One thing to be aware of: in XTend, a function can be called either using the first parameter as a parameter or as the receiver object on which the function is invoked.. What this means is that in the formatEntity function on lines 202-208, line 205 is a call to the format method for ConceptualComposition on lines 229-231. How do I know this? Because the c on line 205 is a ConceptualComposition. How do I know that? Because I've read the UDDL specification and know that the composition collection in a ConceptualEntity contains instances of ConceptualComposition. This kind of thing occurs throughout the UddlFormater.xtend code. Keep it in mind if you're trying to figure out what the code is doing.

maytheu · 2023-09-14T18:32:40Z

Noted

maytheu · 2023-09-14T19:38:38Z

So far i have updated the format() as suggested and check if isConceptualAssociation(), isConceptualEntity() and isConceptualBasisEntityisConceptualBasisEntity() and callinng its respective method.

I have tried to format ConceptualBasisEntity in formatConceptualBasisEntity(), I don't know if that the way to go for formatting ConceptualBasisEntity.

steve-hickman-epistimis · 2023-09-14T20:52:18Z

Reviewing the Langium formatting doc, and the examples ( compare domain-model.langium with domain-model-formatter.ts ), you can call every formatting routine directly in format(AstNode) as though that type of AstNode were at the top level because "The AbstractFormatter calls this method for every node of our model". If you do this then you must always treat each node as though all surrounding context has been set separately - and then ensure that you write the code that way. What does that mean? It means that any time you want to format something with respect to the surrounding context, you must treat it as though the surrounding context has already been formatted and you can only control the position/ formatting of the current node relative to that.

Another alternative is to manage the hierarchy yourself. This is the approach I attempted to describe previously (My apologies if I didn't do a good job above.) For that approach, you need to review the UDDL Spec (publicly available here - see also here ), Chapter 5 or look at the universal-data-definition-language.langium file to understand what is at the top level and what is contained in something else. If you look at the .langium file, you see the entry rule on line 6. This is the top level for every file. The rules referred to in the entry rule indicate what is 'contained' in that. And so on. This tells you where you should call each format method.

Don't do both - pick one approach and stick with it. Attempting to do both will result in some nodes being formatted more than once, which will be slower, and could result in conflicts that are hard to debug.

The approach of calling everything directly from the format(AstNode) method is probably the best approach because that method will be called for every node anyway.

maytheu · 2023-09-14T21:11:06Z

Thanks for the headsup and clarification

maytheu · 2023-09-18T13:12:06Z

I faced an issue previously where the tests failed when I didn't have a formatter for the UDDL definitions. However, after adding UDDL definitions to the formatter file, the test file passed successfully. Surprisingly, when I commented out the changes I made in the formatter file, the tests still passed. This behavior is unexpected, and I'm looking for guidance to understand what might be causing it and where I might be overlooking something.

steve-hickman-epistimis

See comments for an explanation of why the comments had no effect on the tests passing.

steve-hickman-epistimis · 2023-09-18T15:29:53Z

src/language-server/universal-data-definition-language-formatter.ts

-        if(ast.isConceptualAssociation(node)){            
-            this.formatConceptualAssociation(node)
-        }
+        // test passes with this commetned check


The test you have defined does not include a ConceptualAssociation, so commenting this method out will have no effect on test success/ failure.

steve-hickman-epistimis · 2023-09-18T15:49:55Z

src/language-server/universal-data-definition-language-formatter.ts

-        }
+
+        // test passes with this commetned check
+        // if(ast.isConceptualBasisEntity(node)){


The test you have defined only has 1 ConceptualBasisEntity. The formatting you see around it is caused by 2 things: 1) the surrounding formatting is done by formatObj for the containing CDM; 2) A ConceptualBasisEntity is also a ConceptualElement. So when you comment out this method, the formatConceptualElement will still be called.

if you want to make sure you know the effect of a formatting method, you should have multiple instances of that type in your test. In this case, it means including multiple ConceptualBasisEntity instances. At a minimum, have 3. It may also make sense to have instances of different types intermingled where possible just to check on the effect of formatting one type on adjacent instances of a different type.

This brings up an important point: You can't just have if checks in the top level format(AstNode) method. If you do that, then every if is checked for each node and every if check that returns true will result in a format method being called - in the order they show up in format(AstNode). If multiple format methods are called for the same AstNode,not only will that be slower than necessary but also only the last one called will matter, because it will overwrite the effects of all format methods previously called for that AstNode.

Instead, you need to make sure to use if ... else if so that only a single formatting method is called for each AstNode. And you need to make sure to put the formatting methods in order from most specific to most general based on the hierarchy described in the .langium file, because you want the first test to pass to be the most specific formatter applicable.

steve-hickman-epistimis · 2023-09-19T23:09:58Z

uddl_test/uddl_test_formatting/universal-data-definition-language-formatter.test.ts

+    cdm Conceptual "Need to start at Conceptual Level"
+    {
+


Do you really want all these blank lines after formatting? You've got a blank line after every '{' and '}'. That's going to make some files a lot longer - which will force users to scroll. When people have to scroll, they forget what they can't see - which makes it harder to understand complex content.

The test would not pass without the blank lines in the after block

When a test does not pass, you have a choice: change the test (what you did) or change the code being tested. The point I'm making above is that the formatter currently inserts those blank lines - which is why your test needs the blank lines to pass. My question is: do you want the formatter to do that? The goal of formatting is to put the text into a format that is easy to read and understand. Every decision you make about how the formatter works should be made with that in mind.

steve-hickman-epistimis · 2023-09-19T23:13:06Z

uddl_test/uddl_test_formatting/universal-data-definition-language-formatter.test.ts

+
+  it('should format cdm with conceptual basis',async () => {


While you can have tests for each individual type, you may find that a test that combines multiple types is sufficient.

In a previous commit, I merged conceptual entities and conceptual associations into one test case. During this process, only formatter for conceptual entities was checked. I then attempted to verify whether I would encounter similar behavior for conceptual basis and conceptual domain as well.

steve-hickman-epistimis · 2023-09-19T23:15:11Z

uddl_test/uddl_test_formatting/universal-data-definition-language-formatter.test.ts

+  it("should format cdm with conceptual entity", async () => {
+    await universalDataDefinitionLanguageFormatting({
+      before:
+        'dm PPT "Base data structures to support People, Places and Things"{cdm Conceptual "Need to start at Conceptual Level"{centity AddressableEntity "Any entity that is addressable in some way" {};}}',


In your current tests, all the 'before' text is on a single line. You should also have the same tests where the 'before' text has each token on a separate line (you can do this with a find/replace that replaces blanks the before text with CRs).

steve-hickman-epistimis

formatContainer should only be used to format containers, not the individual elements within a container.

steve-hickman-epistimis · 2023-09-29T19:49:14Z

src/language-server/universal-data-definition-language-formatter.ts

+        else if(ast.isLogicalEntity(node)){
+            this.formatLogicalEntity(node)
+        }
+        else if(ast.isLogicalAssociation(node)){


All LogicalAssociations are also LogicalEntities => Swap lines 55-57 and 58-60 so you check for LogicalAssociation first

steve-hickman-epistimis · 2023-09-29T19:51:48Z

src/language-server/universal-data-definition-language-formatter.ts

@@ -36,7 +36,43 @@ export class UniversalDataDefinitionLanguageFormatter extends AbstractFormatter
        }
        else if(ast.isConceptualEntity(node)){
            this.formatConceptualEntity(node)
-        }                
+        }             
+        else if(ast.isLogicalEnumerated(node)){


Why did you choose the ordering of if-else if ... here? Make sure to document it. You don't want someone else to come along after you and make some change to the ordering that causes problems.

steve-hickman-epistimis · 2023-09-29T19:53:01Z

src/language-server/universal-data-definition-language-formatter.ts

+        else if(ast.isPlatformStruct(node)){
+            this.formatPlatformStruct(node)
+        }
+        else if(ast.isPlatformAssociation(node)){


All PlatformAssociations are also PlatformEntities. The check isPlatformAssociation must be before the check isPlatformEntity .

steve-hickman-epistimis · 2023-09-29T19:54:01Z

src/language-server/universal-data-definition-language-formatter.ts

+    protected formatLogicalEnumerated(lenum: ast.LogicalEnumerated): void {
+        this.formatContainer(lenum)
+        lenum.label.forEach(label => {
+            this.formatContainer(label)


Why are you calling formatContainer on label? It's not a container.

The label is an array of LogicalEnumeratedSet | LogicalEnumerationLabel which are both containers

It might be a LogicalEnumeratedSet, which is a container, or a LogicalEnumerationLabel, which is not a container. In these circumstances, remember that the top level format method will figure out the type of label - so all you need to do is call format, which will then call the appropriate formatter for either LogicalEnumerationLabel or LogicalEnumeratedSet. Your current approach will not handle multiple levels of nesting appropriately.

steve-hickman-epistimis · 2023-09-29T19:57:02Z

src/language-server/universal-data-definition-language-formatter.ts

+
+    protected formatLogicalMeasurementSystem(sys: ast.LogicalMeasurementSystem): void {
+        this.formatContainer(sys);
+        sys.constraint.forEach(sysConst => {


measurementSystemAxis is also a collection.

steve-hickman-epistimis · 2023-09-29T22:09:03Z

src/language-server/universal-data-definition-language-formatter.ts

+    protected formatLogicalMeasurementSystemAxis(sysAxis: ast.LogicalMeasurementSystemAxis): void {
+        this.formatContainer(sysAxis);
+        sysAxis.constraint.forEach(sysConst => {
+            this.formatContainer(sysConst);


Individual constraints are not containers.

steve-hickman-epistimis · 2023-09-29T22:09:57Z

src/language-server/universal-data-definition-language-formatter.ts

+    protected formatLogicalReferencePoint(refPoint: ast.LogicalReferencePoint): void {
+        this.formatContainer(refPoint);
+        refPoint.referencePointPart.forEach(pointPart => {
+            this.formatContainer(pointPart);


An individual ReferencePointPart is not a container

steve-hickman-epistimis · 2023-09-29T22:20:22Z

src/language-server/universal-data-definition-language-formatter.ts

+    protected formatLogicalMeasurement(measure: ast.LogicalMeasurement): void {
+        this.formatContainer(measure);
+        measure.attribute.forEach(attr => {
+            this.formatContainer(attr)


Individual attributes are not containers

steve-hickman-epistimis · 2023-09-29T22:20:35Z

src/language-server/universal-data-definition-language-formatter.ts

+            this.formatContainer(attr)
+        })
+        measure.constraint.forEach(mesConst => {
+            this.formatContainer(mesConst)


individual constraints are not containers

steve-hickman-epistimis · 2023-09-29T22:20:57Z

src/language-server/universal-data-definition-language-formatter.ts

+    protected formatLogicalEntity(entity: ast.LogicalEntity): void {
+        this.formatContainer(entity);
+        entity.composition.forEach(comp => {
+            this.formatContainer(comp);


Individual composition elements are not containers

Thanks for the heads up, will make the changes and revert. I will get back if I am stuck.

steve-hickman-epistimis · 2023-10-04T23:36:48Z

Make sure to post your test results each time you want a review. They can be a screenshot of you running the tests and them passing. Or attach a file with the results. The point here is that I should be able to verify that tests pass without running them myself.

maytheu · 2023-10-05T08:46:35Z

Yes, I want to write all test and make sure it passes before taking screenshot

maytheu · 2023-10-09T19:23:49Z

steve-hickman-epistimis

You're making steady progress.

steve-hickman-epistimis · 2023-10-09T21:12:41Z

src/language-server/universal-data-definition-language-formatter.ts

+    protected formatLogicalEnumerated(lenum: ast.LogicalEnumerated): void {
+        this.formatContainer(lenum)
+        lenum.label.forEach(label => {
+            this.formatContainer(label)


It might be a LogicalEnumeratedSet, which is a container, or a LogicalEnumerationLabel, which is not a container. In these circumstances, remember that the top level format method will figure out the type of label - so all you need to do is call format, which will then call the appropriate formatter for either LogicalEnumerationLabel or LogicalEnumeratedSet. Your current approach will not handle multiple levels of nesting appropriately.

steve-hickman-epistimis · 2023-10-09T21:15:43Z

uddl_test/uddl_test_formatting/universal-data-definition-language-formatter.test.ts

+        csa logicalCoordinate "Logical system axis";
+        coord GeographicCoordinates "Represents geographic coordinates" {
+
+            axis:


I suggest that the prompt and its value be on the same line ( axis: "WGS84"). That will be easier to understand.

initial formatter schema

795b99c

maytheu requested a review from steve-hickman-epistimis September 11, 2023 15:15

conceptual entity formatter

a4d238e

formatter for conceptual element

4517414

.

16d7123

test passes with no formmater

39b3605

steve-hickman-epistimis reviewed Sep 18, 2023

View reviewed changes

improve test cases

cfe58ae

steve-hickman-epistimis reviewed Sep 19, 2023

View reviewed changes

maytheu added 3 commits September 28, 2023 16:55

remove extra lines from test

731cd27

logical model formatter

5c61e26

platform data model formatter

7693f72

steve-hickman-epistimis requested changes Sep 29, 2023

View reviewed changes

maytheu added 2 commits October 2, 2023 14:13

format association before entities

f2333d4

ldm test

5a56353

formatting platform element

f196456

steve-hickman-epistimis requested changes Oct 9, 2023

View reviewed changes

initial formatter schema #12

Are you sure you want to change the base?

initial formatter schema #12

Conversation

maytheu commented Sep 11, 2023

steve-hickman-epistimis commented Sep 11, 2023

maytheu commented Sep 12, 2023

steve-hickman-epistimis commented Sep 12, 2023 • edited Loading

maytheu commented Sep 14, 2023

maytheu commented Sep 14, 2023

steve-hickman-epistimis commented Sep 14, 2023 • edited Loading

maytheu commented Sep 14, 2023

maytheu commented Sep 18, 2023

steve-hickman-epistimis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steve-hickman-epistimis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steve-hickman-epistimis commented Oct 4, 2023

maytheu commented Oct 5, 2023

maytheu commented Oct 9, 2023

steve-hickman-epistimis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steve-hickman-epistimis commented Sep 12, 2023 •

edited

Loading

steve-hickman-epistimis commented Sep 14, 2023 •

edited

Loading