Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

feat(GraphQLProperty): support applying variables in the property annotation #18

Closed

Conversation

chemdrew
Copy link
Contributor

some changes discussed as a part of #16

@coveralls
Copy link

coveralls commented Jul 11, 2018

Pull Request Test Coverage Report for Build 23

  • 13 of 13 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 98.922%

Totals Coverage Status
Change from base Build 20: 0.2%
Covered Lines: 459
Relevant Lines: 464

💛 - Coveralls

@chemdrew
Copy link
Contributor Author

@jeremygiberson please review if you have a chance

@@ -25,4 +25,5 @@

String name();
GraphQLArgument[] arguments() default {};
GraphQLVariable[] variables() default {};

Choose a reason for hiding this comment

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

Because variables are now definable w/ @GraphQLProperty -- they are definable at class level now through GraphQLProperty only. Do you want to update @GraphQLVariable(s) annotation to class level as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! I was going to add it here first and then another PR for both the GraphQLArgument and GraphQLVariable to be definable at the class level.
Just wanted to make this PR first for all the logic to handle a class level definition of it

Choose a reason for hiding this comment

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

Okay.

@GPiotr
Copy link

GPiotr commented Jan 15, 2020

Hello there!
May I ask why this PR was never merged into master?
It could be very useful!

@chemdrew chemdrew closed this Jun 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants