-
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
Validator::validateModel doesn't catch resets with variables outside the parent component #294
Comments
A contextual note here would be that resets and their implementation are still under discussion so the reset validation hasn't been pushed very far. Yes I realise that I haven't answered the question. |
To actually answer the question, yes it should raise an error. |
OK, thanks Hugh :) Might put this one further down the list until discussions are done |
Definitely a good choice there, I have pretty much left resets as is for a while. |
Just wanted to check in on this? How are the discussions about resets going ...? |
I believe the current plan is to get 0.9 out the door without any (minimal?) support for resets - that will still make sure that libCellML is able to support the vast majority of CellML 1.0/1.1 models that currently exist. And I think we really need that working implementation that we can use to test "experimental" support for resets to help us evaluate the various options for defining the resets in the specification. |
Yes let's release 0.9 as resets are (baring any major errors) and then look to generate models with reset and see what happens when we mix them with solvers. |
So just to tie up the original issue - should I add tests to the validator now which catch this error or leave it until after 0.9? |
Syntactically I think resets are probably done! Will have this wrapped up in a few weeks hopefully.... |
For 0.9, I would have validation fail if a reset is found with a message about resets not being (currently) supported. |
Hi again all
Changes are in #304 now. Two related things I noticed:
|
For 0.9, I would remove anything that is related to resets in our validation code. Also, I wouldn't explicitly mention that resets "are not supported prior to version 1.0". I would simply say that they "are not currently supported". We clearly want to support them in version 1.0, but better cover our back... :)
Agreed, we should do something about our messages and make them consistent. Maybe this could be addressed as part of issue #314?
Yes, that's a more general issue that I have discussed with @hsorby and @nickerso, and that is related to our current object model. I basically think we need to review it (e.g. a |
|
Added the validator and tests for this issue into PR #355 |
Re: 12.1.1.1 "The value of the variable must be a variable reference to a variable defined within the component element parent of the reset element". Should the validateModel function catch this (added to
TEST(Validator, resets)
)Currently the call to validate the m2 model returns no errors, but I would have expected something to come back? Have I missed something?
The text was updated successfully, but these errors were encountered: