search

cellml / libcellml

Repository for libCellML development.
https://libcellml.org
Apache License 2.0
17 stars 21 forks source link

Analyser/Generator: added some `xxxAsString()` methods #1131

Closed agarny closed 1 year ago

agarny commented 1 year ago

Fixes #446.

agarny commented 1 year ago

I think this is looking ok. The one concern I have is that the use of all the typeToString.at() will throw an exception if called with a non-existent type. I think we have previously agreed to avoid throwing exceptions - maybe? Should we either be protecting from throwing exceptions or documenting that they could happen?

Those typeToString.at() calls are made within methods that take an enum class value as a parameter which is then used as a parameter to the at() method. So, we will always be fine.

nickerso commented 1 year ago

I think this is looking ok. The one concern I have is that the use of all the typeToString.at() will throw an exception if called with a non-existent type. I think we have previously agreed to avoid throwing exceptions - maybe? Should we either be protecting from throwing exceptions or documenting that they could happen?

Those typeToString.at() calls are made within methods that take an enum class value as a parameter which is then used as a parameter to the at() method. So, we will always be fine.

doh! thanks. Is that enforced across to the Python bindings too?

agarny commented 1 year ago

I think this is looking ok. The one concern I have is that the use of all the typeToString.at() will throw an exception if called with a non-existent type. I think we have previously agreed to avoid throwing exceptions - maybe? Should we either be protecting from throwing exceptions or documenting that they could happen? Those typeToString.at() calls are made within methods that take an enum class value as a parameter which is then used as a parameter to the at() method. So, we will always be fine. doh! thanks. Is that enforced across to the Python bindings too?

Hmm, good question! I would guess that it isn't. I mean, enum class values are converted to integers in Python, right? So... anything could go there. @hsorby?

hsorby commented 1 year ago

Yes, I think we have protections on some of the API for the bindings covering enumerations: see here https://github.com/cellml/libcellml/blob/f34f4d9854733f119e2d4bc8bde6933809d6b498/src/bindings/interface/types.i#L119 I think this should be added for these methods as well.

agarny commented 1 year ago

Yes, I think we have protections on some of the API for the bindings covering enumerations: see here

https://github.com/cellml/libcellml/blob/f34f4d9854733f119e2d4bc8bde6933809d6b498/src/bindings/interface/types.i#L119

I think this should be added for these methods as well.

Ok, I believe I have added everything that is needed for our Python bindings. I have also checked for our JavaScript bindings and it looks like we are already all good for that one.

agarny commented 1 year ago

I know we don't test test our Python bindings but in the case of the typeAsString methods and their ilk I think we should test the extents in the Python bindings as this is something that we have added to the interface files to (hopefully) avoid segfaults.

I believe it has already been done, not only for Python (e.g., https://github.com/cellml/libcellml/pull/1131/files#diff-6217397c5913cb67ab08329d9d5b36e17a6a2367ac49c585ae580aa1fd11c328), but also for JavaScript (e.g., https://github.com/cellml/libcellml/pull/1131/files#diff-b49b859ee2ff441946db1f6ea8d135398596fccd8b5b023b6811c7e37b253535)... or am I missing something?