Skip to content

Conversation

enikao
Copy link
Contributor

@enikao enikao commented Feb 11, 2024

No description provided.

Copy link

github-actions bot commented Feb 11, 2024

Test Results

694 tests  ±0   694 ✅ ±0   12s ⏱️ ±0s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit fb74246. ± Comparison against base commit 40c9bb1.

♻️ This comment has been updated with latest results.

@ftomassetti
Copy link
Contributor

I just looked at the diagram you shared on slack and I do not understand why there are subclasses of IFinding that seems to indicates a severity (IError, IInfo, IWarning) but IFinding has also a field of type ISeverity`.

I would consider making severity an enum, as in this way one could write applications that deal with the expected level of severity, that are all predefined. While if we have an interface called ISeverity I suspect people could define arbitrary severities and that may be difficult to handle.

@enikao
Copy link
Contributor Author

enikao commented Feb 12, 2024

You've immediately found the biggest issue with my draft (-:

The severity is indeed duplicated, as I was unsure how to represent it myself (I changed it forth and back a few times).

I think there are two independent issues here:
A) Do we represent severity as field or interface?
B) How do we represent the field values?

A) Do we represent severity as field or interface?

Currently it's a mixture: IWarning essentially pre-populates the field.

The advantage of using interfaces to represent severity is that it's completely in the structure, no instance level needed. This simplifies model management -- we need only a language, no "accessory model".

The advantage of a field is the the general preference "composition over inheritance", and more flexibility. This highly relates to B).

B) How do we represent the field values?

Currently, an ISeverity has a value: integer property. It's meant to establish a total order between severities.
I pre-defined these instances:

Error    value=800000
Warning  value=500000
Info     value=300000

The advantage over an enum is the space we leave for other severities, e.g. Fatal value=900000 or Trace value=200000.
We can still interpret every severity in three categories, depending on their value.

Of course, we could also say "everything should fit into one of these three buckets" -- then an enum would fit perfectly.

@joswarmer
Copy link

I would choose to represent it using a field. We can use either a enum property if we think this is a fixed list. Otherwise I would opt for a string property with as list of predefined values. That opens up the possibility to use additional values. Not sure whether we should.

I don't like the integer property, it is basically meaningless and the order is in my opinion irrelevant.

@joswarmer
Copy link

Location language: - Separating out the location language is a good thing. The location language should become a separate language on its own, useful to reuse in e.g. the LionWeb Differ, and probably other places as well.

@joswarmer
Copy link

Recoverability: - I think we should not say anything about recoverability in the language. What is recoverable depends very much on the application using LionWeb.

@dslmeinte
Copy link
Contributor

Recoverability: - I think we should not say anything about recoverability in the language. What is recoverable depends very much on the application using LionWeb.

But for certain classes of issues – i.e.: the low-level JSON ones – we certainly can say something about recoverability. (Maybe we should “finalize” the correctness document as much and soon as possible?)

@joswarmer
Copy link

Recoverability: - I think we should not say anything about recoverability in the language. What is recoverable depends very much on the application using LionWeb.

But for certain classes of issues – i.e.: the low-level JSON ones – we certainly can say something about recoverability. (Maybe we should “finalize” the correctness document as much and soon as possible?)

But for certain classes of issues – i.e.: the low-level JSON ones – we certainly can say something about recoverability.

Even if we can, we should not say anything about it in the validation language. If we want to say anything at all I would put this in the validation correctness document only.

The validation language should be concise, only reporting the findings.

@joswarmer
Copy link

As with severity, I would also put the level (Serialization, Hierarchical, metaStructural, …) in a (enum or string) property.

@joswarmer
Copy link

Two presentations points:

  • Leaving out Node would make diagrams more readable (less line)
  • In typescript an optional property is shown with a question mark postfix. Using this instead of <> might make the diagrams easier to read (less space needed).

@enikao
Copy link
Contributor Author

enikao commented Feb 12, 2024

Two presentations points:

I agree, and removed Node from all the manually edited diagrams.

@enikao
Copy link
Contributor Author

enikao commented Feb 12, 2024

Recoverability: - Even if we can, we should not say anything about it in the validation language. If we want to say anything at all I would put this in the validation correctness document only.

The validation language should be concise, only reporting the findings.

It's not in the core validation language, but the serialization validation language. I agree with Meinte, it makes sense in that context.
IMHO, this should be technically accessible. I want to write code like

if (findings.all(f => f instanceof ICompletelyRecoverable)) {
  // happily continue working with my chunk
}

@ftomassetti
Copy link
Contributor

I don't like the integer property, it is basically meaningless and the order is in my opinion irrelevant.

I would also argue that enum literals have an order of definition within the enum declaring that, and we could use that order

@ftomassetti
Copy link
Contributor

As with severity, I would also put the level (Serialization, Hierarchical, metaStructural, …) in a (enum or string) property.

This is what we do in StarLasu. Each issue has an IssueType (one of Lexical, Syntactic, Semantic, Translation) and an IssueSeverity (one of Info, Warning, Error). We have been using this for a few years and so far it worked well for us

@joswarmer
Copy link

It's not in the core validation language, but the serialization validation language

Not sure what you mean by "core validation language: vs "serialization validation language"? Can you explain?

@joswarmer
Copy link

But for certain classes of issues – i.e.: the low-level JSON ones

These cases are usually either already recovered by the JSON parser or marked as an error by the JSON parser used. We decided to just follow the JSOIN spec, so these errors are not even LionWeb errors.

@joswarmer
Copy link

if (findings.all(f => f instanceof ICompletelyRecoverable)) {
// happily continue working with my chunk
}

Two points:

  • Happily continue is not quiet right, it says "recoverable", not "already recovered", so the application still need to decide whether it can recover this error and how it would do this.
  • In TypeScript you cannot use instanceof for interfaces, so this won't work. Even if we add recoverable to the language I think it should be a property with an enum value.

@enikao
Copy link
Contributor Author

enikao commented Feb 13, 2024

Not sure what you mean by "core validation language: vs "serialization validation language"? Can you explain?

"core validation language" is the base of all validation (reporting). A client would ask for the derived model by this language to get all validations (as we've decided a long time ago to identify derived models by their language). It defines very basic classifiers used in any kind of validation.

"serialization validation language" is an extension of the former. It's used specifically to report issues found in serialization.

@enikao
Copy link
Contributor Author

enikao commented Feb 13, 2024

But for certain classes of issues – i.e.: the low-level JSON ones

These cases are usually either already recovered by the JSON parser or marked as an error by the JSON parser used. We decided to just follow the JSOIN spec, so these errors are not even LionWeb errors.

The errors would still need to be reported. And all error reports are nodes, so they need a concept.

@enikao
Copy link
Contributor Author

enikao commented Feb 13, 2024

if (findings.all(f => f instanceof ICompletelyRecoverable)) {
// happily continue working with my chunk
}

In TypeScript you cannot use instanceof for interfaces, so this won't work. Even if we add recoverable to the language I think it should be a property with an enum value.

I didn't mean I want to write exactly that code, but want a way to filter for e.g. completely recoverable findings.

@joswarmer
Copy link

I didn't mean I want to write exactly that code, but want a way to filter for e.g. completely recoverable findings.

As I argued before, I don't think recoverability is a findings property, it is a property of the application whether it can or cannot recover from something.

@joswarmer
Copy link

But for certain classes of issues – i.e.: the low-level JSON ones

These cases are usually either already recovered by the JSON parser or marked as an error by the JSON parser used. We decided to just follow the JSOIN spec, so these errors are not even LionWeb errors.

The errors would still need to be reported. And all error reports are nodes, so they need a concept.

I agree that they should be reported, but the discussion here is about whether JSON findings are recoverable or not.

@joswarmer
Copy link

In general, I consider "marker" interfaces (those without any features) as an anti-pattern. I know they are heavily used in MPS, but I never liked them. I would like to replace all the marker interfaces with properties (as has already been discussed for all of them somewhere above(.

@enikao
Copy link
Contributor Author

enikao commented Feb 13, 2024

Changes in latest commits:

  • Split location into separate language
  • Use enum for Severity
  • Use enum for SerializationSeverity

Now the advantage of marker interfaces is clear -- I can't know from the language which severity each finding is.

@joswarmer
Copy link

Added n alternative version of the Findings (and related) languages in the folder docs/alternativeusing enums instead of interfaces. There is a README.md explaining the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants