Skip to content

support_repeatable_directives#1986

Closed
dugenkui03 wants to merge 3 commits intographql-java:support_repeatable_directivesfrom
dugenkui03:support_repeatable_directives
Closed

support_repeatable_directives#1986
dugenkui03 wants to merge 3 commits intographql-java:support_repeatable_directivesfrom
dugenkui03:support_repeatable_directives

Conversation

@dugenkui03
Copy link
Copy Markdown
Contributor

No description provided.

@dugenkui03 dugenkui03 force-pushed the support_repeatable_directives branch from f4b9d93 to 7280bc4 Compare July 23, 2020 15:33
@dugenkui03 dugenkui03 force-pushed the support_repeatable_directives branch from 7280bc4 to aff7c04 Compare July 23, 2020 15:47
@andimarek andimarek added the breaking change requires a new major version to be relased label Aug 25, 2020
@andimarek
Copy link
Copy Markdown
Member

Note: this is a breaking change.

Copy link
Copy Markdown
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

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

We are looking to go into a slightly different direction on this.

We need much of this code (and we need raw grammar support as well)

But I think we will leave the names as is to maximise backwards compatibility (DirectiveContainer vs DirectivesContainer ) and leave the getDirective(name) as a singleton directive fetcher that throws if there is more than 1

We will take some of the code so we appreciate this.

There will be a branch that I will start soon

default Directive getDirective(String directiveName) {
default List<Directive> getDirective(String directiveName) {
return getDirectivesByName().get(directiveName);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think for backwards compatibility I think we should add a getFirstDirective(String name) method. This would get the 0th entry of null if empty

While multiple directives are allowed, I posit that a lot of people will use one mostly.

We also have a choice here - we could make Directive getDirective(String directiveName) be the "get first" and then add a getDirectives(String name) say.

@andimarek - thoughts?

@bbakerman
Copy link
Copy Markdown
Member

See #2015

@bbakerman bbakerman closed this Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change requires a new major version to be relased

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants