-
Notifications
You must be signed in to change notification settings - Fork 46
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
Generate _key methods for attributes #109
Conversation
Also some more thoughts: to avoid possible intersections by user-defined has_enumeration_for :foo, with: FooEnum, create_key_helper: true and in addition to that we may allow to configure default options, smth like EnumerateIt.configure do |config|
config.default_options = { create_key_helper: true }
end or just EnumerateIt.configure do |config|
config.create_key_helper = true
end @lucascaton Let me know what do you think :) |
@lucascaton would appreciate it if you could take a look :) |
I will, just give me some time, please :)
|
Hi @eiskrenkov 👋 Firstly, thanks for your PR 🙂
My pleasure! I'm so glad to hear that it's been helpful 😃
I'll have to fix that first, then I'll get back to your PR |
Well, I had to skip SQLite v2 for now (PR #110), as it may be a bug in the gem... ... but at least that won't block your PR anymore 🙂 |
I like the idea! 😃 Thanks for submitting this PR. Could you please rebase it with the SQLite3 change I made, so we get tests passing again?
That somehow works for me locally and on GitHub Actions 😅 but you're right, I think that should be explicit! All good to keep that :) |
That's not really necessary, for two reasons:
|
Never mind, I managed to do it myself :) |
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.
Nice! Thanks again @eiskrenkov 🙌
I've released a new version of the gem with this change: |
Oh it seems like I missed all the fun, sorry for that! Yeah, I'm ok with not doing this as an opt-in feature as long as you're fine with it :) Thank you so much for releasing it, appreciate your help, have a great day! ❤️ |
For anyone stumbling upon this PR, here's a custom rubocop cop that restricts # lib/rubocop/cop/your_namespace/enumerate_it_avoid_key_for.rb
module RuboCop
module Cop
module YourNamespace
# Checks for key_for method calls on EnumerateIt::Base subclasses.
# Use `attribute_name_key` instead.
# See. https://github.com/lucascaton/enumerate_it/pull/109
#
# @safety
# Will find out i guess
#
# @example
# # bad
# EnumClass.key_for(record.value)
#
# # good
# record.value_key
#
class EnumerateItAvoidKeyFor < Base
extend AutoCorrector
requires_gem 'enumerate_it', '>= 3.3.0'
METHOD_NAME = :key_for
MSG = "Prefer using `attribute_name_key` instead of `#{METHOD_NAME}` on EnumerateIt::Base subclasses".freeze
# BaseEnumeration is here in case you have your base enumeration class inherited
# from `EnumerateIt::Base`, you can just remove it if you don't
def_node_matcher :enumerate_it?, <<~PATTERN
{
(const {nil? cbase} :BaseEnumeration)
(const (const {nil? cbase} :EnumerateIt) :Base)
}
PATTERN
def on_send(node) # rubocop:disable Metrics/CyclomaticComplexity
return if node.receiver.nil? && !ancestor_inherits_enumerate_it_base?(node)
return unless node.method_name == METHOD_NAME && node.arguments.one?
argument = node.arguments.last
return unless argument.respond_to?(:dot?) && argument.dot? && !argument.parenthesized_call?
add_offense(node, message: MSG) do |corrector|
corrector.replace(node, "#{argument.receiver.source}.#{argument.method_name}_key")
end
end
private
def ancestor_inherits_enumerate_it_base?(node)
node.each_ancestor(:class).any? { |class_node| enumerate_it?(class_node.parent_class) }
end
end
end
end
end The implementation is quick and dirty, but it just works™️ :) Simply require it in your require:
- ./lib/rubocop/cop/your_namespace/enumerate_it_avoid_key_for.rb |
Hey there! First of all, thank you so much for maintaining this gem, love it!
Im my company we're heavy users of
enumerate_it
and we're having a ton of enum classes. Let's cosider the following example:We're using numbers as values to save the planet and some space in our db
Later, somewhere in the code to get the human readable value back we need to use
key_for
on the enum class like thatSo although our model's attribute is already tied to a specific enum class, we need to type the constant name again, calling
key_for
on it. Not only it requires to always remember the class name of enum which is associated with the attribute, but also can lead to mistakes, when accidentally usingkey_for
on wrong enum (e.g both of them have integers as values in associated hash that are starting from 0)So my proposal would be to generate
#{attribute_name}_key
methods similar to#{attribute_name}_humanize
to simplify things:This way the enum user code will look cleaner and will be free of accidental mistakes related to the usage of wrong enum classes
The implementation seems pretty straightforward, but for sure i may be missing something. Please, let me know what do you think, thanks a lot in advance!
P.S I added
activerecord
andsqlite
to development dependencies as specs are not running for me locally without doing so.spec_helper
requiresactive_record
, so I assume this is the right thing to do :)