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

Drop require_nested and include_concern #656

Merged

Conversation

jrafanie
Copy link
Member

Zeitwerk completely removes the need for these methods. See also: ManageIQ/manageiq#22854

@miq-bot
Copy link
Member

miq-bot commented Jan 26, 2024

Checked commits jrafanie/manageiq-providers-ovirt@212a55e~...6143cff with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
21 files checked, 0 offenses detected
Everything looks fine. 🍰

@agrare agrare self-assigned this Jan 26, 2024
@@ -1,8 +1,5 @@
class ManageIQ::Providers::Ovirt::Inventory::Collector < ManageIQ::Providers::Inventory::Collector
# TODO: review the changes here and find common parts with ManageIQ::Providers::Ovirt::InfraManager::Inventory::Strategies::V4
Copy link
Member

Choose a reason for hiding this comment

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

This comment was attached to those require lines... Not sure if it should move somewhere else

Copy link
Member Author

Choose a reason for hiding this comment

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

We have similar/copied lines here: https://github.com/ManageIQ/manageiq-providers-red_hat_virtualization/pull/40/files#diff-f2f5d4e887f15b36afacf467c592be51b1b1055f4aa04d5c553f77042620c671R2

Not sure what it's referring to but I can come back to it when I finish up the other PRs. Maybe it was prior code that was removed and left orphaned. https://twitter.com/nzkoz/status/538892801941848064

@agrare
Copy link
Member

agrare commented Jan 26, 2024

@jrafanie do these PRs have any dependencies? On ManageIQ/manageiq#22854 or anything else?

@jrafanie
Copy link
Member Author

@jrafanie do these PRs have any dependencies? On ManageIQ/manageiq#22854 or anything else?

@agrare We've been running with zeitwerk by default since ManageIQ/manageiq#22488, so that's the dependency.

Technically, here is where we dropped support for the classic autoloader (merged about 1 month ago), making usage of these methods no longer needed: ManageIQ/manageiq#22801

@jrafanie
Copy link
Member Author

@jrafanie do these PRs have any dependencies? On ManageIQ/manageiq#22854 or anything else?

@agrare We've been running with zeitwerk by default since ManageIQ/manageiq#22488, so that's the dependency.

Technically, here is where we dropped support for the classic autoloader (merged about 1 month ago), making usage of these methods no longer needed: ManageIQ/manageiq#22801

Updated ManageIQ/manageiq#22854 with this information.

@agrare agrare merged commit 580042d into ManageIQ:master Feb 6, 2024
3 checks passed
@jrafanie jrafanie deleted the drop_require_nested_include_concern branch February 6, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants