-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add ability to detect the model class implicitly. #8
Add ability to detect the model class implicitly. #8
Conversation
lib/graphql/sugar/object.rb
Outdated
common_attribute_names = [:id, :created_at, :updated_at] | ||
common_attribute_names.each(&method(:attribute)) | ||
def initialize_common_attributes | ||
COMMON_ATTRIBUTES.each do |_attribute| |
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.
Rename _attribute
to common_attribute
- As per our style guide, we prefix with underscore for unused vars: https://github.com/rubocop-hq/ruby-style-guide#underscore-unused-vars
end | ||
|
||
def attribute(name, return_type_expr = nil, desc = nil, **kwargs, &block) | ||
raise "You must define a `model_class` first in `#{self.name}`." if @model_class.blank? | ||
|
||
initialize_common_attributes if @attributes_initialized.exclude?(name) && !all_initial_attributes_added? | ||
@attributes_initialized.add(name) |
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 would simplify to:
return if @attributes_initialized.include?(name)
@attributes_initialized.add(name)
- Since model_class is no longer required, we need to call initialize_common_attributes in both
attribute
andmodel_class
.
initialize_common_attributes
- Remove the
all_initial_attributes_added?
method as the check would be present within theattribute
method itself.
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.
-
the above check for the
@attributes_initialized
is to prevent the recursion and would create the duplicate types. This is just the mechanism to control the recursion when we want to run theintialize_common_attributes
. -
We still need the
model_class
method in those scenarios when the Automatic type detection failed. See: https://github.com/Amandeepsinghghai/sample-graphql-app/blob/tmp-work/app/graphql/types/optional_user_type.rb#L3 -
I am all ears, on this, I also felt that this can improved.
lib/graphql/sugar/object.rb
Outdated
common_attribute_names.each(&method(:attribute)) | ||
def initialize_common_attributes | ||
COMMON_ATTRIBUTES.each do |_attribute| | ||
@attributes_initialized.add(_attribute) |
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.
Remove @attributes_initialized.add(_attribute)
from within initialize_common_attributes
as that it is being handled by attribute
method.
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 need to add the attribute to the @attributes_initialized
Set, so that it won't be called again from the attribute
method.
Part of #7
Now while defining new types, if you the automatic detection will first try to detect the class upon the code initialization.
BEFORE
AFTER