-
-
Notifications
You must be signed in to change notification settings - Fork 909
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
feat: support alias in matcher define_enum #1419
feat: support alias in matcher define_enum #1419
Conversation
Do we have an issue related? :) |
Do we need one? I ran into this when I was creating some aliases for a legacy table in a database. |
It would be good to keep tracking and also describe what you're solving. It could be both a bugfix and a new feature. |
Makes sense to me. @mcmire thoughts? I can describe it on an issue if you want. |
This is fine. I appreciate the updates to the documentation because I could figure out pretty quickly what this was about. No need for an issue :) |
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.
One little thing, but this makes sense otherwise! I like how simple this PR is.
@@ -370,7 +376,10 @@ def expected_column_type | |||
end | |||
|
|||
def column | |||
model.columns_hash[attribute_name.to_s] | |||
key = attribute_name.to_s | |||
column_name = model.attribute_aliases[key] || key |
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.
It looks like attribute_aliases
is not a public method in the API, but attribute_alias
is: https://api.rubyonrails.org/classes/ActiveModel/AttributeMethods/ClassMethods.html#method-i-attribute_alias. What do you think about using that instead?
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 updated it. good catch! thanks
@mcmire would you mind reviewing it again? 😃 |
Sorry for the delay. I was waiting until I had confirmation on the plan for the next version, but it seems like we've got that settled now. Looks good, merging now! |
No description provided.