-
Notifications
You must be signed in to change notification settings - Fork 61
Rename internal variables and methods to start with _
#304
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
base: master
Are you sure you want to change the base?
Conversation
The type hints were misleadingly saying int, when in fact subprocess.run accepts float timeouts.
8255e68
to
d798e71
Compare
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. |
@arun3688 can you please answer the above questions and review this PR. |
Based on PR #312 the method |
@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 |
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 acommandLineOptions
arg in the constructor.)To minimize future merge conflicts, and for me to be able to take advantage of
pre-commit
while making these changes, this PR is based on #289.