-
Notifications
You must be signed in to change notification settings - Fork 900
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
Remove return value sync_workers/sync_starting_workers #22942
Remove return value sync_workers/sync_starting_workers #22942
Conversation
3352164
to
d0970fc
Compare
d0970fc
to
5e1d97d
Compare
I thought these were used for logging purposes somewhere (maybe not the starting_workers, but the sync_workers results. I recall logging with something showing the current and desired and then which ones it's starting and which ones it's taking down to get to the desired state. EDIT: I might be thinking of this: manageiq/app/models/mixins/per_ems_worker_mixin.rb Lines 24 to 28 in e1eb9f4
|
result = klass.sync_workers | ||
result[:adds].each { |pid| worker_add(pid) unless pid.nil? } |
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.
Naming it result is confusing, since we aren't actually returning it anymore. Think this is clearer to read:
result = klass.sync_workers | |
result[:adds].each { |pid| worker_add(pid) unless pid.nil? } | |
workers = klass.sync_workers | |
workers[:adds].each { |pid| worker_add(pid) unless pid.nil? } |
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.
There's an instance-var and attr_reader named workers
so I think a local var named the same is also confusing
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.
Oh ok, but I still think result is confusing... w
or synced_workers
could also work... Naming is hard 😅
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 will go with synced_workers then
@Fryguy I think you're talking about https://github.com/ManageIQ/manageiq/blob/master/app/models/mixins/per_ems_worker_mixin.rb#L24 which is in |
Haha yeah I just edited my last comment 😅 I thought that was higher up for some reason. |
Checked commits agrare/manageiq@49ffc18~...a5c4698 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
These return values don't appear to be used anywhere and make the code slightly more complicated.
Related to: #22941