Skip to content

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jul 30, 2025

We may need to cross repo w/ a fix for include

@miq-bot cross-repo-test /all

end

ActiveRecord::Base.include VirtualAttributes::VirtualTotal
ActiveSupport.on_load(:active_record) { include VirtualAttributes::VirtualTotal }
Copy link
Member

Choose a reason for hiding this comment

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

Should it be this in the block: ActiveRecord::Base.include VirtualAttributes::VirtualTotal?

Copy link
Member Author

@kbrock kbrock Aug 7, 2025

Choose a reason for hiding this comment

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

@jrafanie I totally worried about this.

I have gone over this in my mind many times.
I ran the code with a few options:

  • commenting this out (base case to ensure it was doing something)
  • AS.include(AR::VT) (original)
  • AS.on_load { AS.include(AR::VT) }
  • AS.on_load { include(AR::VT) }

ruboco complains about both of the AS.include(AR::VT) cases.

Maybe I should just drop that change.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I don't mind fixing warnings that are trivial. This one doesn't feel trivial. Let's get the others in on their own.

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

just a single question, everything else looks good

@kbrock
Copy link
Member Author

kbrock commented Aug 8, 2025

update:

  • dropped the active_support includes cop fix.

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM

@jrafanie jrafanie merged commit 2640e74 into ManageIQ:master Aug 19, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants