{"action":"created","issue":{"url":"https://api.github.com/repos/cellml/libcellml/issues/118","repository_url":"https://api.github.com/repos/cellml/libcellml","labels_url":"https://api.github.com/repos/cellml/libcellml/issues/118/labels{/name}","comments_url":"https://api.github.com/repos/cellml/libcellml/issues/118/comments","events_url":"https://api.github.com/repos/cellml/libcellml/issues/118/events","html_url":"https://github.com/cellml/libcellml/issues/118","id":161860426,"number":118,"title":"Current thinking on validation","user":{"login":"dladd","id":811296,"avatar_url":"https://avatars.githubusercontent.com/u/811296?v=3","gravatar_id":"","url":"https://api.github.com/users/dladd","html_url":"https://github.com/dladd","followers_url":"https://api.github.com/users/dladd/followers","following_url":"https://api.github.com/users/dladd/following{/other_user}","gists_url":"https://api.github.com/users/dladd/gists{/gist_id}","starred_url":"https://api.github.com/users/dladd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dladd/subscriptions","organizations_url":"https://api.github.com/users/dladd/orgs","repos_url":"https://api.github.com/users/dladd/repos","events_url":"https://api.github.com/users/dladd/events{/privacy}","received_events_url":"https://api.github.com/users/dladd/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":28,"created_at":"2016-06-23T07:48:38Z","updated_at":"2016-07-22T11:02:29Z","closed_at":null,"body":"In a meeting at the ABI on Tuesday, there was some discussion about how we might implement validation as the next step in the use-cases doc.\r\n\r\nThe team here was in favour of **(A)** setting up a `Validator` object, similar to how we deal with the `Parser` in the changeset under review in #114 . Consistent with this, it was also suggested that we should probably move our serialisation routines into a separate `Serialiser` object (we might want to call this `Printer` or something else instead), rather than doing serialisation from the objects themselves. \r\n\r\nFor example, a user wishing to check if a model is valid, output it if so or output errors if not might do:\r\n```c++\r\n    libcellml::Validator validator;\r\n    libcellml::Serialiser serialiser(libcellml::Format::XML);\r\n    validator.validate(model);\r\n    if (validator.hasErrors()) {\r\n        for (size_t i = 0; i < validator.errorCount(); ++i) {\r\n            std::cout << validator.getError(i)->getDescription();\r\n        }\r\n    } else {\r\n        std::cout << serialiser.serialise(model);\r\n    }\r\n```\r\n\r\nAn alternative **(B)** might be to do validation on entities as we currently do for serialisation, for example:\r\n```c++\r\n    model->validate();\r\n    if (model->hasErrors()) {\r\n        for (size_t i = 0; i < model->errorCount(); ++i) {\r\n            std::cout << model->getError(i)->getDescription();\r\n        }\r\n    } else { \r\n        std::cout << model->serialise(libcellml::Format::XML);\r\n    }\r\n```\r\nWhile **B** is a bit more concise, the thinking is that it is worth the extra line or two to make our reading, writing, and validation error logging consistent. And  `Parser` may need to pick up errors that do not necessarily translate into an Entity (e.g. unrecognised or mismatched XML tags).\r\n\r\n<br>\r\nWith the validator, there is also the question of when/if to automatically clear errors. For instance, consider if we call the following validation on two very simple models:\r\n```c++\r\n    libcellml::ModelPtr m1 = std::make_shared<libcellml::Model>();\r\n    libcellml::ModelPtr m2 = std::make_shared<libcellml::Model>();\r\n    libcellml::Validator validator;\r\n    validator.validate(m1);\r\n    validator.validate(m2);\r\n    m1->setName(\"model1\");\r\n    validator.validate(m1);\r\n```\r\n`validator` might now contain the following errors regarding the requirement for valid models to have names:\r\n- **(1)** no errors if we clear validation errors every time `validate()` is called.  \r\n- **(2)** errors from `m1` and `m2` if we allow validation errors to persist until the user calls `validator.clearErrors()`\r\n- **(3)** an error from `m2` only if we remove any previous errors pertaining to the object being validated from the `validator` and add back any new errors from the updated object.  \r\n\r\nI don't have a strong preference but would probably go with **3**."},"comment":{"url":"https://api.github.com/repos/cellml/libcellml/issues/comments/234516495","html_url":"https://github.com/cellml/libcellml/issues/118#issuecomment-234516495","issue_url":"https://api.github.com/repos/cellml/libcellml/issues/118","id":234516495,"user":{"login":"agarny","id":602265,"avatar_url":"https://avatars.githubusercontent.com/u/602265?v=3","gravatar_id":"","url":"https://api.github.com/users/agarny","html_url":"https://github.com/agarny","followers_url":"https://api.github.com/users/agarny/followers","following_url":"https://api.github.com/users/agarny/following{/other_user}","gists_url":"https://api.github.com/users/agarny/gists{/gist_id}","starred_url":"https://api.github.com/users/agarny/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/agarny/subscriptions","organizations_url":"https://api.github.com/users/agarny/orgs","repos_url":"https://api.github.com/users/agarny/repos","events_url":"https://api.github.com/users/agarny/events{/privacy}","received_events_url":"https://api.github.com/users/agarny/received_events","type":"User","site_admin":false},"created_at":"2016-07-22T11:02:29Z","updated_at":"2016-07-22T11:02:29Z","body":"@nickerso, I indeed had something like that in mind, but... then the question is whether we want  libCellML users to be able to access those methods? I thought the answer was no and that it was the reason the `Unit` structure is declared in the `units.cpp` and not in `units.h`.\r\n\r\nSo, if we don't mind libCellML users to have access to those methods, then your suggestion looks fine to me. On the other hand, if we don't want libCellML users to have access to them, then we would have no other choice but to make `Validator` a friend of `Units`."}}
{"actor":{"display_login":"agarny"}}