Skip to content
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

Fixes #37431 - Fix Style/SoleNestedConditional cop #10068

Merged

Conversation

archanaserver
Copy link
Contributor

Reviewed and optimized nested conditionals flagged by the Style/SoloNestedConditional cop.

Copy link
Contributor

@nofaralfasi nofaralfasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failing tests appear to be related to your changes. Additionally, after applying these changes, there is a need to fix the indentations.

lib/net/dhcp/record.rb Outdated Show resolved Hide resolved
@archanaserver archanaserver force-pushed the cop-style/solecestedconditional branch from a0bdc37 to 6aab33d Compare May 9, 2024 11:07
@archanaserver archanaserver changed the title Fix Style/SoleNestedConditional cop Fixes #37431 - Fix Style/SoleNestedConditional cop May 9, 2024
@archanaserver archanaserver force-pushed the cop-style/solecestedconditional branch from 6aab33d to 968cf2d Compare May 9, 2024 11:10
@archanaserver
Copy link
Contributor Author

@ekohl there is one rubocop offense detected that is Lint/Syntax https://github.com/theforeman/foreman/actions/runs/9093977252/job/24994075137?pr=10068, even though syntax seems correct to me when i checked the file. Do you mind checking the failure here?

@archanaserver archanaserver force-pushed the cop-style/solecestedconditional branch from 8ce6b1d to 808856e Compare June 24, 2024 20:53
@archanaserver archanaserver force-pushed the cop-style/solecestedconditional branch 2 times, most recently from 45e5ba8 to 5dc1322 Compare August 1, 2024 08:16
app/controllers/concerns/foreman/controller/users_mixin.rb Outdated Show resolved Hide resolved
@@ -173,11 +173,9 @@ def dhcp_update_required?
(!old.build? && build? && !all_dhcp_records_valid?))
# Handle jumpstart
# TODO, abstract this way once interfaces are fully used
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this TODO isn't actually solved with 4cbf879. Probably not for this PR, but something to investigate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried doing something here #10261

ekohl
ekohl previously approved these changes Aug 1, 2024
@ekohl
Copy link
Member

ekohl commented Aug 1, 2024

@nofaralfasi have your concerns been addressed?

nofaralfasi
nofaralfasi previously approved these changes Aug 4, 2024
@archanaserver
Copy link
Contributor Author

@ekohl, can we get this in?

@ehelms ehelms merged commit b1cc7eb into theforeman:develop Oct 16, 2024
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants