Skip to content

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

Open
wants to merge 46 commits into
base: master
Choose a base branch
from

Conversation

ondras12345
Copy link
Contributor

@ondras12345 ondras12345 commented Jun 13, 2025

As suggested in #254

I wasn't sure about these, so I left them alone:

  • Should isParameterChangeable() be part of the public API?
  • Should setTempDirectory() be part of the public API?
  • Should 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 a commandLineOptions 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.

@ondras12345 ondras12345 marked this pull request as draft June 16, 2025 18:00
@ondras12345
Copy link
Contributor Author

Converted to draft, probably best to wait for #289 and #295 before fixing merge conflicts here.

@ondras12345 ondras12345 marked this pull request as ready for review June 19, 2025 16:17
@ondras12345
Copy link
Contributor Author

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 adeas31 requested a review from arun3688 June 25, 2025 12:48
@adeas31
Copy link
Member

adeas31 commented Jun 25, 2025

@arun3688 can you please answer the above questions and review this PR.

@syntron
Copy link
Contributor

syntron commented Jun 25, 2025

Based on PR #312 the method isParameterChangeable() is needed as part of the public API!

@syntron
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants