-
Notifications
You must be signed in to change notification settings - Fork 919
[WIP] Switch ActsAsArModel to be QueryRelation based by default #23618
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
base: master
Are you sure you want to change the base?
Conversation
| end | ||
|
|
||
| singleton_class.send(:alias_method, :find_by_id, :lookup_by_id) | ||
| Vmdb::Deprecation.deprecate_methods(singleton_class, :find_by_id => :lookup_by_id) |
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 deprecation was moved into the base class.
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.
I don't think we use find_by_id anymore. I did find in a few spots, but they look rare.
Maybe your search shows more callers (to any class)
df9c3b9 to
084b6ec
Compare
084b6ec to
1471303
Compare
|
Interestingly no deprecations showed up in the tests...let's see what cross repo shows @miq-bot cross-repo-test /all |
From Pull Request: ManageIQ/manageiq#23618
| def self.to_legacy_options(options) | ||
| def self.from_legacy_options(options) | ||
| { | ||
| :conditions => options[:where], | ||
| :include => options[:includes], | ||
| :limit => options[:limit], | ||
| :order => options[:order], | ||
| :offset => options[:offset], | ||
| :select => options[:select], | ||
| :group => options[:group], | ||
| :where => options[:conditions], | ||
| :includes => options[:include], | ||
| :limit => options[:limit], | ||
| :order => options[:order], | ||
| :offset => options[:offset], | ||
| :select => options[:select], | ||
| :group => options[:group], | ||
| }.delete_blanks |
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.
yea, think we can drop support for find(:all).
We have no callers and only class implements this interface
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.
Think we can be more yolo here.
The interface for QueryRelation is search. We decided to convert that to legacy find, for all of our AAArM implementations, but since PglogicalSubscription is the only class that implemented find(), lets just fix it in the one place and not keep around the legacy find support and all.
I am a little concerned that Chargebacks and VimPerformanceTrends do not properly implement the AAArM interface, but it looks like reports call a custom method to run those reports. (i.e.: trend.rb )
Don't feel we should keep dead weight in our interface when we don't have anything using that interface (except for pglogical)
|
Yeah I was concerned maybe something was using that old interface that's why I added the deprecations but nothing showing up as deprecations so I don't think it's used |
|
This pull request is not mergeable. Please rebase and repush. |
kbrock
left a comment
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 great change. It would be a shame not get it in.
| # | ||
| def self.find(*_args) | ||
| raise NotImplementedError, _("find must be implemented in a subclass") | ||
| def self.find(mode_or_id, *args) |
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.
Really think we can drop this. The only place we use it is in PgLogical (as mentioned in the previous list). This will remove legacy_options and a lot of other stuff. (Not sure the legacy options even work in current versions of Rails.)
| end | ||
|
|
||
| singleton_class.send(:alias_method, :find_by_id, :lookup_by_id) | ||
| Vmdb::Deprecation.deprecate_methods(singleton_class, :find_by_id => :lookup_by_id) |
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.
I don't think we use find_by_id anymore. I did find in a few spots, but they look rare.
Maybe your search shows more callers (to any class)
This flips ActsAsArModel to be search-first (i.e. via QueryRelation) and deprecates the old-style find method that takes :all, :first, or :last (However, find with an id is still supported just like in Rails). This also deprecates the old-style find_by_id and find_all_by_id.
WIP: I think I should change the default implementation of
PglogicalSubscription#searchto handlewhere :id- right now it can't handle it.cc @kbrock