Skip to content

Fix AST::fromArray conversion of initial values (match initial to expected conversion)#724

Merged
vladar merged 2 commits intowebonyx:masterfrom
zimzat:fix-directive-definition-locations-list-type
Sep 16, 2020
Merged

Fix AST::fromArray conversion of initial values (match initial to expected conversion)#724
vladar merged 2 commits intowebonyx:masterfrom
zimzat:fix-directive-definition-locations-list-type

Conversation

@zimzat
Copy link
Contributor

@zimzat zimzat commented Aug 31, 2020

  • Parse DirectiveDefinitionNode->locations as NodeList<NamedNode>
  • Parse Parser::implementsInterfaces as NodeList<NamedTypeNode>
  • Update signature of Parser::unionMemberTypes to match actual NodeList<NamedTypeNode>

Fixes #723

Note: I wasn't sure how to address the phpstan error so I updated the baseline. I can revisit this if necessary.

 ------ ----------------------------------------------------------------------- 
  Line   src/Validator/Rules/KnownDirectives.php                                
 ------ ----------------------------------------------------------------------- 
         Ignored error pattern #^Only booleans are allowed in a negated         
         boolean, array<string\>\|null given\.$# in path                        
         /home/zimzat/files/sources/graphql-php/src/Validator/Rules/KnownDirec  
         tives.php was not matched in reported errors.                          
  102    Only booleans are allowed in a negated boolean, array|null given.      
 ------ ----------------------------------------------------------------------- 

…ected conversion)

- Parse `DirectiveDefinitionNode->locations` as `NodeList<NamedNode>`
- Parse `Parser::implementsInterfaces` as `NodeList<NamedTypeNode>`
- Update signature of `Parser::unionMemberTypes` to match actual `NodeList<NamedTypeNode>`

Fixes webonyx#723
@coveralls
Copy link

coveralls commented Aug 31, 2020

Coverage Status

Coverage decreased (-0.009%) to 86.3% when pulling f0b997d on zimzat:fix-directive-definition-locations-list-type into 4f34309 on webonyx:master.

Copy link
Collaborator

@spawnia spawnia left a comment

Choose a reason for hiding this comment

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

This makes the Parser more self-consistent, looks good to me.

@zimzat
Copy link
Contributor Author

zimzat commented Aug 31, 2020

Thanks @spawnia. I removed instead of updated the @return because other places didn't have them and nothing was complaining, though I agree there's benefit in having them. 👍

@zimzat
Copy link
Contributor Author

zimzat commented Aug 31, 2020

@spawnia It looks like phpcs marks those as duplicate information errors 😞

Should I remove them again, or is there something I could do in PHPCS to allow for them?

@spawnia
Copy link
Collaborator

spawnia commented Sep 1, 2020

Should I remove them again, or is there something I could do in PHPCS to allow for them?

Maybe we can fix that in the future, let's remove it for now to keep this PR focused.

@zimzat zimzat force-pushed the fix-directive-definition-locations-list-type branch from d3dc845 to 3373240 Compare September 1, 2020 11:53
@zimzat
Copy link
Contributor Author

zimzat commented Sep 1, 2020

Done 👍 Thanks!

@vladar vladar merged commit af15467 into webonyx:master Sep 16, 2020
@vladar
Copy link
Member

vladar commented Sep 16, 2020

Thanks for the fix!

@zimzat zimzat deleted the fix-directive-definition-locations-list-type branch September 16, 2020 16:58
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.

AST::fromArray converts DirectiveDefinitionNode->locations from array into NodeList

5 participants