-
Notifications
You must be signed in to change notification settings - Fork 21
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
Creating Issue class to replace Error class #521
Conversation
Makes it easier to understand: we have the cause (the type of object with an error) and the level (fatal, warning, info)
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 far the only changes are renaming By default, all the pre-existing error situations remain as I'm imagining additional use cases / tests / implementation of:
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 :) |
Should we extend the |
I'm not a fan of the |
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>
.
|
|
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 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?
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 ...? |
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.
It's geared towards validation in the sense that we have methods like
I just don't see why we should be able distinguish between As for creating a |
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
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. |
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.
Yes, I guess it's the way forward. |
( @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). |
Just a note that the error message from |
Done in commit c96cc7c. |
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.
Let's merge this monster!
Not quite... :) |
Should have left include re-org for another PR. |
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.
Let's do it she's good to go, merge the monster!
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! :) |
None from me :) |
(changes, or miracles) ;) |
In this time and day: miracles, no doubt! :) |
Woo hoo, 2 approvals. Time to merge and find out how much it impacts PR #572! |
(whew) Thanks all :) |
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:
... anything else ... ?