Uh oh!
There was an error while loading. Please reload this page.
Rename internal variables and methods to start with _ - #304
Conversation
ondras12345
commented
Jun 16, 2025
8255e68 to
d798e71Compareondras12345
commented
Jun 19, 2025
Rebased and fixed all conflicts. If the answer to all 3 questions in the original comment is 'yes', then this PR is ready for review and merge. If not, I'll rename the functions to indicate that they are internal. |
adeas31
commented
Jun 25, 2025
@arun3688 can you please answer the above questions and review this PR. |
syntron
commented
Jun 25, 2025
Based on PR #312 the method |
syntron
commented
Jun 25, 2025
@ondras12345 just as a side note: if these variables are modified / renamed - would it be possible to change them to lowercase, with words separated by underscores? I have tried to not touch such cases as I'm not sure if there is any rule regarding this within OMPython. However, here we would change it to private and, thus, have all possibilities to change it without touching a public API |
ondras12345
commented
Jul 2, 2025
Done. |
adeas31
left a comment
There was a problem hiding this comment.
Look good.
From myside it is fine to leave the isParameterChangeable(), setTempDirectory()and setCommandLineOptions() as part of public API.
Uh oh!
There was an error while loading. Please reload this page.
As suggested in #254
I wasn't sure about these, so I left them alone:
isParameterChangeable()be part of the public API?setTempDirectory()be part of the public API?setCommandLineOptions()be part of the public API? (I.e. should it be allowed to set extra options after the object is constructed? We already have acommandLineOptionsarg in the constructor.)To minimize future merge conflicts, and for me to be able to take advantage of
pre-commitwhile making these changes, this PR is based on #289.