Skip to content
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

Restore type_for(type) function #1714

Merged
merged 2 commits into from
Jan 10, 2024
Merged

Conversation

antoine-malliarakis
Copy link
Contributor

@antoine-malliarakis antoine-malliarakis commented Nov 9, 2023

Motivation

Following #1622 the function Tapioca::Dsl::Helpers::GraphqlTypeHelper#type_for required an GraphQL::Schema::Argument (instead of a GraphQL::Schema::Wrapper) to be passed.

Though this did make perfectly sense, we actually used that method to explicitly compute types in our DSL process without this being necessarily "for a field". For example it could be for union types.

Implementation

What I did was:

  1. Rework the type_for method to be backward compatible with the pre Update GraphQL mutation compiler to consider arguments with replace_null_with_default #1622 code (e.g. keep accepting a sole GraphQL::Schema::Wrapper argument as a parameter)
  2. Introduce an explicit type_for_argument which would be redirecting appropriately to keep Update GraphQL mutation compiler to consider arguments with replace_null_with_default #1622 feature

Tests

I added tests for some introduced adherence.

@antoine-malliarakis antoine-malliarakis marked this pull request as ready for review November 10, 2023 10:04
@antoine-malliarakis antoine-malliarakis requested a review from a team as a code owner November 10, 2023 10:04
Comment on lines +29 to +51
T.class_of(GraphQL::Schema::Scalar),
T.class_of(GraphQL::Schema::Enum),
T.class_of(GraphQL::Schema::Union),
T.class_of(GraphQL::Schema::Object),
T.class_of(GraphQL::Schema::Interface),
T.class_of(GraphQL::Schema::InputObject),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could have put

  • a simple T.class_of(GraphQL::Schema::Member)
  • a T.untyped

But, the first option is referring to a "private API" which I'm not really fond of, and the second option seems to be missing the point...

Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation looks good and thank you for the tests.

I'm not 100% sure if we should support this but I'm leaning towards yes. I'll wait for another reviewer to confirm it's okay before merging.

@antoine-malliarakis antoine-malliarakis marked this pull request as draft January 2, 2024 09:50
@antoine-malliarakis
Copy link
Contributor Author

Flagging as draft as there were other refactorings which made this PR actually outdated. Will rework.

@antoine-malliarakis antoine-malliarakis marked this pull request as ready for review January 2, 2024 10:29
@antoine-malliarakis
Copy link
Contributor Author

@KaanOzkan rebased at head to take into account latest modifications. Not too impacting it seems.

@vinistock do you think you could have a look at this PR eventually ? (Or is there someone else I would need to contact to that extent ?)

Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. There are some CI failures though

… to sorbet it should only be a symbol but according to the doc it passes)
@antoine-malliarakis
Copy link
Contributor Author

Changes look good to me. There are some CI failures though

I believe I fixed the linting (or more specifically, the type check error). I'm not too sure about how to reproduce the errors reported for the other ruby versions though. Still checking.

@KaanOzkan
Copy link
Contributor

Failures were unrelated to this PR

@KaanOzkan KaanOzkan added the refactor Code refactor with no behaviour change label Jan 10, 2024
@KaanOzkan KaanOzkan merged commit 317491d into Shopify:main Jan 10, 2024
17 checks passed
@antoine-malliarakis antoine-malliarakis deleted the chore/restore-type_for branch January 16, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code refactor with no behaviour change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants