Creating Issue class to replace Error class

[kerimoyle]

As discussed in various threads:
#314
#368
#493
#514

... we'd like to have an Issue class to replace the current Error class, in order to allow the return of non-fatal error messages and more helpful warnings and information. This would include situations in which the cellml models does not violate the specifications but is still nonsense (would not run as a simulation, contains mismatched units in the mathematics, or mismatched multipliers in the units, etc).

It's also a good place to review the construction of our descriptions to ensure that they're all consistent and helpful.

Here's the current list of things which need a non-fatal message to be added:

• multiplier mismatch in equivalent units
• multiplier mismatch in maths
• units mismatch in maths
• parsed model parts which will later cause validation errors (?) as in Remove validation code form parser. #302
• cyclic or recursive imports (?) as in Validation should check for recursive imports #280
• exponents with units (should be dimensionless) as in Wishlist of functions from writing tutorials #509
• maths statements which are not assignments (ie: valid MathML/CellML but not runable)
• anything which will cause issues in the Generator, as in Segfaults in generator #492, Code generation: ensure that a model is runnable #493, Refactoring: extract the model analysis from the code generator #499,

... anything else ... ?

[kerimoyle]

This is very clearly not finished yet, but I wanted to ask for feedback on the structure etc now before I go through and add the checks for issues which will populate the non-fatal issue categories.
So ... @agarny @nickerso @hsorby @awickens ... pre-review please :)

So far the only changes are renaming Error class to Issue (and all the fallout from that). Within the Issue class, I've renamed Issue::Kind to be Issue::Cause (to be clearer about what we mean now that there's a second enum) and introduced the new Issue::Level (ERROR, WARNING, HINT) enum.

By default, all the pre-existing error situations remain as Level::ERRORs using the addError() function and the default Level::ERROR.

I'm imagining additional use cases / tests / implementation of:

• returning, in order, combinations of levels (eg: errors and warnings) as discussed in Replace the Error class with the Issue class with a type (e.g. error, warning, hint) #314
• storing somehow a reference to more information for things which aren't necessarily related to the specification, eg: mathlm that's valid cellml but not runable, or generator stuff. Ideally this should be a reference to a heading number in the informative spec, which should probably in turn relate to the formal spec too? See thoughts on Specifications document as *rst cellml-specification#25
• ... anything else?

Next step is then to add checks for non-fatal issues throughout (see list above) and make sure that the references make sense.

Thoughts? Thanks :)

[hsorby]

Should we extend the Issue::Level enum to have (SPECIFICATION_ERROR, ERROR, WARNING, HINT) enum. This may give us the flexibility to disclose errors that arise for the generator but are not violations of the spec.

[hsorby]

I'm not a fan of the HINT issue level, I would prefer INFO.

[kerimoyle]

Haha, it was originally INFO and I changed it back ...

…

On Wednesday, 15 January 2020, Hugh Sorby ***@***.***> wrote: I'm not a fan of the HINT issue level, I would prefer INFO. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#521?email_source=notifications&email_token=ALFXCNAOWQXGXZKWOIMTTWDQ55VRXA5CNFSM4KHBSOU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJBVNPQ#issuecomment-574838462>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALFXCNEPXVQJ74QPDZRK2FDQ55VRXANCNFSM4KHBSOUQ> .

[agarny]

• I don't understand why the Logger class has an addError(), addWarning() and addHint() method on top of the addIssue() method. Surely, addIssue() ought to be enough, since an Issue can be either an error, a warning or a hint?
• I prefer HINT to INFO (this is what I am used from compilers).
• I would definitely not have SPECIFICATION_ERROR. Our Issue class must remain generic while this would make it too specifc.
• Right now, the Issue class is very much geared towards validation, which is not good. Remember, we may have other types of issues (e.g. issues related to code generation). So, I would probably have a very generic Issue class and then probably a ValidationIssue class. In a similar way, we could have a CodeGenerationIssue class, if deemed necessary.

[kerimoyle]

I don't understand why the Logger class has an addError(), addWarning() and addHint() method on top of the addIssue() method. Surely, addIssue() ought to be enough, since an Issue can be either an error, a warning or a hint?

I originally went through and changed them to be addIssue() calls, but reverted so that the format was closer to what was there before. I only made the different methods because I wanted to forestall people asking for them ;)

I would definitely not have SPECIFICATION_ERROR. Our Issue class must remain generic while this would make it too specifc.
Right now, the Issue class is very much geared towards validation, which is not good. Remember, we may have other types of issues (e.g. issues related to code generation).

I don't quite follow how the Issue class is geared towards validation? (except that we haven't been able to add anything other than validation errors yet?) As far as I can see they're all as generic as it could be?

So, I would probably have a very generic Issue class and then probably a ValidationIssue class. In a similar way, we could have a CodeGenerationIssue class, if deemed necessary.

Also not sure how making a distinction between SPECIFICATION_ERROR and ERROR would affect the operation too much - I'd have thought that the creation of a whole separate class was way less flexible! Given that enums are pretty simple, we could also go for both ...?
Personally I think we should stick with error levels that are agnostic of their cause - if it's an ERROR, it's a fatal error whatever its reason. It's up to you (or perhaps the user?) to decide if the presence of non-runnable-stuff in the generator is an ERROR or a WARNING? I'd probably go for ERROR in that situation.

[agarny]

I don't understand why the Logger class has an addError(), addWarning() and addHint() method on top of the addIssue() method. Surely, addIssue() ought to be enough, since an Issue can be either an error, a warning or a hint?
I originally went through and changed them to be addIssue() calls, but reverted so that the format was closer to what was there before. I only made the different methods because I wanted to forestall people asking for them ;)

But... at the end of the day, we are still adding an issue, no matter whether it's actually an error, a warning or a hint, so really no point in those additional methods. If anything, I find them confusing.

Right now, the Issue class is very much geared towards validation, which is not good. Remember, we may have other types of issues (e.g. issues related to code generation).
I don't quite follow how the Issue class is geared towards validation? (except that we haven't been able to add anything other than validation errors yet?) As far as I can see they're all as generic as it could be?

It's geared towards validation in the sense that we have methods like rule()/setRule(), specificationHeading(), component()/setComponent() and the like. For a code generation related issue, those methods are not needed.

So, I would probably have a very generic Issue class and then probably a ValidationIssue class. In a similar way, we could have a CodeGenerationIssue class, if deemed necessary.

Also not sure how making a distinction between SPECIFICATION_ERROR and ERROR would affect the operation too much - I'd have thought that the creation of a whole separate class was way less flexible! Given that enums are pretty simple, we could also go for both ...?

I just don't see why we should be able distinguish between SPECIFICATION_ERROR and ERROR. We have Cause to help us for that.

As for creating a ValidationIssue class, that's the way it should be done in object oriented programming. If we think it's too "heavy" then I am happy for us to go with an alternative solution. I guess we could still have all those validation-specific methods. It's just that they would "useless" when it comes to, say, code generation related issues.

[kerimoyle]

It's just that they would "useless" when it comes to, say, code generation related issues.

I don't agree that they're useless. All that needs to happen is that we document somewhere what the generator needs, publish it somewhere with reference numbers, and refer to that in exactly the same way as we refer to the CellML specification document. We could rename specificationHeading to be something more generic, perhaps?

It's geared towards validation in the sense that we have methods like rule()/setRule(), specificationHeading(), component()/setComponent() and the like. For a code generation related issue, those methods are not needed.

For code generation I'd think it would still be useful to the user to have a reference back to the object which caused the problem - after all, it's those objects (the components, variables etc) that the user can change ... so they still need to know where to fix the problem, even if the problem is reported inside the generator. I'll mock up a test or something to show you what I mean.

[agarny]

It's just that they would "useless" when it comes to, say, code generation related issues.
I don't agree that they're useless.

I am talking about the validation-specific methods. Based on the current error messages that the code generator generates, they are not needed. However, as you said at the end of your message, they might be needed based on the kind of error message the code generator generates.

All that needs to happen is that we document somewhere what the generator needs, publish it somewhere with reference numbers, and refer to that in exactly the same way as we refer to the CellML specification document. We could rename specificationHeading to be something more generic, perhaps?

Yes, I guess it's the way forward.

[kerimoyle]

( @agarny Just to say I did have a crack at writing a decent test for the generator, but have given up cos I couldn't find a reasonable example ... urgh).

[kerimoyle]

Just a note that the error message from validateVariable could be upgraded to tell users that they may simply need to link their units to the model via linkUnits(). See #516 (comment)

[agarny]

default.profraw still needs to be deleted.

Done in commit c96cc7c.

[hsorby]

Let's merge this monster!

[agarny]

Let's merge this monster!

Not quite... :)

[hsorby]

Should have left include re-org for another PR.

[agarny]

Should have left include re-org for another PR.

Yes, sorry, couldn't help it... Argh!

Anyway, the PR is ready for re-re-reviewing. Thanks in advance @nickerso and @hsorby!

[hsorby]

Let's do it she's good to go, merge the monster!

[agarny]

Ok, no more changes from me (and probably from @kerimoyle too). So, all good (finally?! :)) for @nickerso and @hsorby to review and merge in... before the end of the day?... I want/need a miracle! :)

[kerimoyle]

None from me :)

[kerimoyle]

(changes, or miracles) ;)

[agarny]

In this time and day: miracles, no doubt! :)

[agarny]

Woo hoo, 2 approvals. Time to merge and find out how much it impacts PR #572!

[kerimoyle]

(whew) Thanks all :)