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

Add support for multiple "argsBuilder" #321

Open
Kocal opened this issue Apr 26, 2018 · 4 comments
Open

Add support for multiple "argsBuilder" #321

Kocal opened this issue Apr 26, 2018 · 4 comments

Comments

@Kocal
Copy link
Member

Kocal commented Apr 26, 2018

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? .
Version/Branch .

As discussed on a Slack thread, that would be awesome to have the possibility to write something like that:

myField:
  argsBuilders: # argsBuilderS, not argsBuilder
    - 
      builder: MyFirstArgsBuilder
      config: ...
    -
      builder: MySecondArgsBuilder
      config: ...

It would prevent to us to write something not flexible at all:

class MyFirstArgsBuilder implements MappingInterface
{
    public function toMappingDefinition(array $config): array
    {
        $definition = [
            'foo' => [
                'type' => 'Int',
            ],
        ];

		return $definition;
    }
}


class MySecondArgsBuilder implements MappingInterface
{
    public function toMappingDefinition(array $config): array
    {
        $definition = [
            'bar' => [
                'type' => 'Int',
            ],
        ];

        return (new MyFirstArgsBuilder())->toMappingDefinition($config) + $definition;
    }
}

Also, it should handle argument name conflicts. For example, it should throw an exception if MyFirstArgsBuilder and MySecondArgsBuilder both provide foo argument.

Thanks!

@mcg-web mcg-web added this to the v0.12 milestone Apr 26, 2018
@akomm
Copy link
Member

akomm commented Apr 8, 2019

@Kocal
What do you expect, when:

  • an args builder emits arg which already defined in configs
  • an successive args builder emits arg which already emitted by previous arg(s) builders?

edit Think for the first its useful to behave like the argsBuilder currently does. The question is whether to propagate this logic to args builderS due to maybe confusing situation when you use multiple builders and they mess each other up. This is why with the ability to add types in builder(s) #473 I went the routine of denying overrides. The question is whether it gets confusing when all the builders behave differently in this case. So take this just as a thought for future and ignore the first point :)

@Kocal
Copy link
Member Author

Kocal commented Apr 9, 2019

What do you expect, when an successive args builder emits arg which already emitted by previous arg(s) builders?

Ah you're right, I didn't think about this.
I didn't touch GraphQL/ArgsBuilders for months, but I have 2 propositions:

  1. Do a simply array_merge with definitions arrays
  2. Modifiy the toMappingDefinition method to inject a new parameter that is the previous mapping definition, and let the user do what he wants:
class MySecondArgsBuilder implements MappingInterface
{
    public function toMappingDefinition(array $config, array $parentDefinition): array
    {
       // Here, `$parentDefinition` is the result of the previous args builder `MyFirstArgsBuilder`

        $definition = [
            'bar' => [
                'type' => 'Int',
            ],
        ];

        return $parentDefinition + $definition;
    }
}

@akomm
Copy link
Member

akomm commented Apr 9, 2019

I would not yet touch the MappingInterface as it is used in too many different places.

Maybe I phrase the focus of the question differently: Do you think an arg builder replacing args from other arg builder is rather not an error and you want to allow it or rather error you want to know about at compile time after changing the config.

@Kocal
Copy link
Member Author

Kocal commented Apr 9, 2019

... I don't know, both solutions seem good to me ¯\(ツ)

@mcg-web mcg-web modified the milestones: v0.12, v0.13 May 26, 2019
@mcg-web mcg-web removed this from the v0.13 milestone Dec 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants