-
Notifications
You must be signed in to change notification settings - Fork 122
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 a json_api_client DSL compiler #1565
Conversation
# empty? does not exist on JsonApiClient::Schema | ||
schema if schema.size > 0 # rubocop:disable Style/ZeroLengthPredicate |
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.
Disabled rubocop rule because it was a false positive
# If the association is broken, it will raise a NameError when trying to access the association_class | ||
klass = association.association_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.
There is a test for this
when ::JsonApiClient::Associations::BelongsTo::Association | ||
# id must be a string: # https://jsonapi.org/format/#document-resource-object-identification | ||
[association.param.to_s, "T.nilable(::String)"] | ||
when ::JsonApiClient::Associations::HasOne::Association | ||
[association.attr_name.to_s, "T.nilable(#{klass})"] | ||
when ::JsonApiClient::Associations::HasMany::Association | ||
[association.attr_name.to_s, "T.nilable(T::Array[#{klass}])"] |
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.
Unfortunately, all of those things can be nullable when an object is created without being loaded via the API
if property.default.nil? | ||
as_nilable_type(sorbet_type) |
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.
Properties with default values are always initialized with their default value, even when creating a bare object.
We made the decision to make those properties non-nilable to make the usage cleaner.
That being said, it is possible today to have a nil:
class User < JsonApiClient::Resource
property :name, type: :string, default: "foo"
end
> u = User.new
=> #<User:@attributes={"type"=>"users", "name"=>"foo"}>
> u.name
=> "foo"
> u.name = 123
=> 123
> u.name # Casts to string
=> "123"
> u.name = nil
> u.name # Actually nil
=> nil
748d749
to
7fc153a
Compare
7fc153a
to
6f4eb56
Compare
Signed-off-by: Alexandre Terrasa <[email protected]>
Signed-off-by: Alexandre Terrasa <[email protected]>
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.
Don't have context on json_api_client
but the compiler mostly LGTM.
I'm not sure if it should be a default compiler inside tapioca though 😕 I'll discuss with the team.
).void | ||
end | ||
def generate_methods_for_association(mod, association) | ||
# If the association is broken, it will raise a NameError when trying to access the association_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.
Should we rescue and rethrow a better error telling the user that the association is broken?
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.
That's what I wanted to do at first, but @Morriar 's argument was that this would show a stack within the gem, with a full stack trace, instead of us catching it. It's a problem of the user misusing the gem, not misusing tapioca/sorbet.
Another point is that this is forward compatible. If the gem changes its implementation, we don't have to change anything.
I'm okay either way, just highlighting the discussion.
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.
We have a way for compilers to signal errors that we use in AR associations compiler, for example:
tapioca/lib/tapioca/dsl/compilers/active_record_associations.rb
Lines 168 to 176 in bc6805e
rescue SourceReflectionError | |
add_error(<<~MSG.strip) | |
Cannot generate association `#{reflection.name}` on `#{constant}` since the source of the through association is missing. | |
MSG | |
rescue MissingConstantError => error | |
add_error(<<~MSG.strip) | |
Cannot generate association `#{declaration(reflection)}` on `#{constant}` since the constant `#{error.class_name}` does not exist. | |
MSG | |
end |
# We let the generation raise, the user should not define an association without a corresponding class. | ||
# The association will be unusable anyway. |
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 problem with raising an exception in a compiler is that the end-user has very little control over the cause of the exception (especially if the code is not under their control, like when the constant is coming from a gem) but their whole DSL generation process is now interrupted, so they can't finish generating all their DSL RBI files.
Motivation
Fixes #1554
This could perhaps be implemented in https://github.com/JsonApiClient/json_api_client as a tapioca extension, but it's worth noting that the gem is used in several places at Shopify and is the spec used by the Vault API, namely used in iHub.
Implementation
The implementation was largely cargo-culted from the
smart-properties
compiler.I paired with @Morriar
See comments for some of the design decisions.