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

Merge guessed argument properties and args described with GQL\Arg #1151

Merged
merged 10 commits into from
Mar 5, 2024

Conversation

mathroc
Copy link
Contributor

@mathroc mathroc commented Jan 12, 2024

Q A
Bug fix? yes/no
New feature? yes/no
BC breaks? yes/no
Deprecations? no
Tests pass? yes/no
Documented? no
Fixed tickets #...
License MIT

When a single argument is described with #[GQL\Arg] all other are not guessed anymore

This commit starts by guessing all arguments and override the ones that have #[GQL\Arg] attributes describing them

I'm not sure of a few things:

  • should the type specified in the attribute override the one that was guessed?
  • same for the default
  • Is there cases where guessing could fail and adding the #[GQL\Arg] attribute was a way to prevent guessing failing / crashing?

in the future it would be nice if we could add the attribute on the argument directly instead of the method so we could make the name & type optional there (or forbidden?)

When a single argument is described with `#[GQL\Arg]` all other are not guessed anymore

This commit starts by guessing all arguments and override the ones that have `#[GQL\Arg]` attributes describing them
@Vincz
Copy link
Collaborator

Vincz commented Jan 12, 2024

@mathroc to answer your questions:

  • Should the type specified in the attribute override the guessed one => YES. The developper should have the final word regarding the argument type. So, #[GQL\Arg] should be the priority.
  • For the default as well.
  • We auto-guess only if we don't have a #[GQL\Arg]

You are right, we could also explore the possibility to set attributes directly on the arguments.

Copy link
Contributor Author

@mathroc mathroc left a comment

Choose a reason for hiding this comment

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

make the attributes have the final word on type and default value

src/Config/Parser/MetadataParser/MetadataParser.php Outdated Show resolved Hide resolved
src/Config/Parser/MetadataParser/MetadataParser.php Outdated Show resolved Hide resolved
@mathroc
Copy link
Contributor Author

mathroc commented Jan 12, 2024

and for the last question, I can answer it by looking at the tests: yes sometimes the guessing can't work at all, e.g.: arrays

🤔

@Vincz
Copy link
Collaborator

Vincz commented Jan 12, 2024

Yes, and we want an error when we don't have a corresponding #[GQL\Arg] and the auto-guess failed.
To make this work, you first need to get arguments having a corresponding#[GQL\Arg] and auto-guess only the remaining ones (by passing the list of already resolved to the auto-guesser for example).

@mathroc
Copy link
Contributor Author

mathroc commented Jan 14, 2024

How should the arguments be ordered? if it has any importance)

@mathroc mathroc marked this pull request as ready for review January 14, 2024 15:03
@Vincz
Copy link
Collaborator

Vincz commented Jan 15, 2024

@mathroc The GraphQL arguments order doesn't matter as they are named. But we need them in the right order when the PHP method is called.

@mathroc
Copy link
Contributor Author

mathroc commented Jan 15, 2024

ok, I think the PR is fine then, let me know if there's anything to do. (should I squash the commits?)

@Vincz Vincz merged commit d416c0d into overblog:master Mar 5, 2024
38 checks passed
@mathroc mathroc deleted the patch-2 branch March 5, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants