-
Notifications
You must be signed in to change notification settings - Fork 130
Small code adjustment when reading the code #6630
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
Conversation
|
jenkins build this please |
cf8606a to
b1dd87a
Compare
|
jenkins build this please |
| /*is_producer=*/true, | ||
| /*injection_phase_not_used=*/Phase::OIL); | ||
| /*is_production_group=*/true, | ||
| /*injection_phase=*/Phase::OIL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a dummy argument for production groups, right? It can be debated if it is more readable to indicate that in the comment or not. I think the original version is more readable here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, fair enough. no strong opinion either. I can revert this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I get a warning with my IDE editor because of the comment different from the function declaration.
I changed it to the following way,
const int num_gr_ctrl = this->groupControlledWells(chain[ii],
/*always_included_child=*/"",
/*is_production_group=*/true,
/*injection_phase=*/Phase::OIL/*not used*/);| /*always_included_child=*/"", | ||
| /*is_producer=*/true, | ||
| /*injection_phase_not_used=*/Phase::OIL); | ||
| /*is_production_group=*/true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to change is_producer to is_production_group here? I think both are readable, so no strong opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just trying to be the same with the function declaration, and other usages in the same file.
int groupControlledWells(const std::string& group_name,
const std::string& always_included_child,
const bool is_production_group,
const Phase injection_phase) const;b1dd87a to
746b7d4
Compare
|
jenkins build this please |
|
Thanks for the review and approval. The firs commit is good to me since there is no loop with the OR operation, it should not be there for the sake of the readability. The second commit is mostly due to not updating the comments when the function declaration was adjusted. |
|
With jenkins passes, I am getting the PR in now. |
hopefully helping the readability.