First review of the Python API #545
Conversation
|
Thanks for reviewing the whole Python API of IODE |
| @@ -469,6 +473,9 @@ cdef class _AbstractDatabase: | |||
|
|
|||
| self.abstract_db_ptr.copy_into(input_files.encode(), objects_names.encode()) | |||
|
|
|||
| # GDM>: | |||
| # * same comment as above (i.e. rename to merge_from?) | |||
| # * what is the difference between merge_into and copy_to? The doctests do not help me understand the difference. | |||
There was a problem hiding this comment.
Honnestly, the french documentation about WsCopy and WsMerge is not clear. I don't know the difference.
@jmpplan Could you explain the difference between WsCopy and WsMerge please?
| @@ -501,6 +508,17 @@ cdef class _AbstractDatabase: | |||
| input_file = str(Path(input_file).resolve()) | |||
| self.abstract_db_ptr.merge_into(input_file.encode()) | |||
|
|
|||
| # GDM> | |||
| # * I think the API would be nicer if that information was available as properties on the "single object" classes | |||
| # (e.g. Equation, Scalar, Table). You might need to create classes for Comment, Identity, List and Variable, | |||
There was a problem hiding this comment.
Not impossible but will require to complexify the implementation of __getitem__ and __setitem__. Not sure the extra work and especialy the future cost maintainance worth to create special Python classes Comment, Identity, List and Variable with only one prorperty.
| @@ -112,6 +112,7 @@ cdef class Equations(_AbstractDatabase): | |||
| elif isinstance(value, Equation): | |||
| equation = value | |||
| elif isinstance(value, dict): | |||
| # GDM> it seems odd to have this code/capability here instead of as an Equation method (e.g. Equation.update(dict) -- I don't know if that also exists) | |||
There was a problem hiding this comment.
I don't really see what is the problem to implement the case where the passed 'value' is a dict in the special method __setitem__ ?
For your information, the __getitem__ and __setitem__ methods are implemented in the super class AbstractDatabase and calls protected method _get_object and _set_object which are implemented in each subclass (Comments, ..., Variables).
There was a problem hiding this comment.
Having some code here is not a problem, but that's a lot of code which work at the level of a single Equation within the Equations class, so I find the implementation should be in an Equation method and here only a call to that method.
| @@ -10,6 +10,10 @@ | |||
|
|
|||
| from reports cimport B_ReportExec, B_ReportLine | |||
|
|
|||
| # GDM> I don't really know how report works, so I don't know if a report loads a workspace inside the report | |||
There was a problem hiding this comment.
At the second you import the Python iode package (or start the new GUI), 7 global databases are created in memory. These databases are destroyed when you quit Python (or the GUI).
The method report_exec below (newly called execute_report) will apply each IODE command contained in the IODE report. The execution of an IODE report (or an IODE command via the Python function execute_command) will NOT create new databases and it does NOT with a separate memory space. With IODE, everything is global (sigh).
So
I don't know if a report loads a workspace inside the report
The answer is no.
if you execute a report once a workspace is already loaded
Yes.
If it is the second option, maybe make this a method on the Workspace class
An IODE report (or command) is not always about workspace. I am trying to stay more or less close to the non-Python API which provide the convenient IODE function ReportExec.
I also think that the users will not understand why the Python functions execute_report and execute_command are attached to a top level Workspace class.
For the moment, I prefer to keep the functions execute_report and execute_command as it.
In the future, if users ask for this, I would be keen to change my mind.
| # GDM> I don't really know how report works, so I don't know if a report loads a workspace inside the report | ||
| # or if you execute a report once a workspace is already loaded. If it is the second option, | ||
| # maybe make this a method on the Workspace class. Otherwise (or maybe either way?), create a Report class and make | ||
| # an execute() method on it. |
There was a problem hiding this comment.
In the C code, there is a C struct REPFILE (see https://github.com/plan-be/iode/blob/master/api/iode.h#L1091) which represents an IODE report and alongside, they are a lot of 'private' and 'public' functions to manipulate this C struct. It would definitly worth to group the C struct REPFILE and the related C functions into a C++ class but that's not for tomorrow.
But yes, a C++Report class would be very welcome to cleanup the C code and help maintainers. But I don't see any real added value a public Python Report class for the users.
| # GDM> as much as I understand this explanation, I think it is not clear | ||
| # enough for inexperienced Python programmers, | ||
| # as I think somebody is bound to write | ||
| # >>> table.insert(index, ["RIDG:", 'RIDG']) |
There was a problem hiding this comment.
From the French documentation of IODE:
Définition des cellules
Pour distinguer une cellule texte d'une cellule LEC, la cellule de texte devra commencer par les doubles guillemets (qui ne seront pas imprimés). Une cellule ne débutant pas par un double guillemet sera considérée comme une forme LEC et syntaxiquement vérifiée.
In other words:
Cell definition
To distinguish a text cell from a LEC cell, the text cell must begin with double quotation marks (which will not be printed). A cell that doesn't start with a double quotation mark will be considered a syntax-checked LEC form.
I know it's weird but long-time IODE users are used to this implicit rule.
|
I will close this PR as I consider I either made the requested change or created a specific issue. |
Congratulations for the great work ! The new API feels more Pythonic than the previous one, and I appreciate the gigantic work this must have been.
There are some areas where I think the object-oriented approach could be taken further. I think it would make the API more consistent.
PS: do NOT merge this PR