Skip to content

Reify AST node types and remove unneeded nullability #751

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jan 23, 2021
Merged

Reify AST node types and remove unneeded nullability #751

merged 10 commits into from
Jan 23, 2021

Conversation

spawnia
Copy link
Collaborator

@spawnia spawnia commented Dec 11, 2020

This achieves a few things:

  • make the code easier to understand both by humans and static analyzers
  • improve performance by eliminating unnecessary null checks
  • improve usability through a clarified API

@spawnia
Reify AST node types and remove unneeded nullability
8afce04 
This achieves a few things:
- make the code easier to understand both by humans and static analyzers
- improve performance by eliminating unnecessary null checks
- improve usability through a clarified API
@coveralls
Copy link

coveralls commented Dec 11, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.005%) to 86.38% when pulling bda44f8 on spawnia:reify-ast-node-types into c4d2f82 on webonyx:master.

@spawnia spawnia mentioned this pull request Dec 11, 2020
Merged
3 tasks
@spawnia spawnia added the bug label Dec 12, 2020
@spawnia spawnia requested a review from simPod January 4, 2021 16:26
simPod
simPod approved these changes Jan 5, 2021
View reviewed changes
Copy link
Collaborator

@simPod simPod left a comment

Choose a reason for hiding this comment

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

👌🏾

spawnia and others added 2 commits January 5, 2021 12:16
@spawnia @simPod
Update src/Utils/ASTDefinitionBuilder.php
b78e6fd 
Co-authored-by: Simon Podlipsky <simon@podlipsky.net>
@spawnia
Update ASTDefinitionBuilder.php
a2566a6
spawnia
spawnia commented Jan 5, 2021
View reviewed changes
vladar
vladar approved these changes Jan 23, 2021
View reviewed changes
Copy link
Member

@vladar vladar left a comment

Choose a reason for hiding this comment

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

Nice change 👍 But do you think it is safe to publish as a patch? I am a bit hesitant. Inclining to include it in the next minor.

@spawnia
Copy link
Collaborator Author

spawnia commented Jan 23, 2021

The actual code changes are very minor and would be best classified as bug fixes. If anything, it would uncover implementations which are already bugged in some way.

@vladar
Copy link
Member

vladar commented Jan 23, 2021

We have conflicts now after merging #740. Mind fixing? Then we can ship it and safely publish it in the new minor 🚢

@spawnia
Merge branch 'master' into reify-ast-node-types
950fb32 
# Conflicts:
#	src/Language/AST/InterfaceTypeDefinitionNode.php
#	src/Language/AST/InterfaceTypeExtensionNode.php
#	src/Utils/ASTDefinitionBuilder.php
@spawnia
Fix interfaces property
bda44f8
@spawnia
Copy link
Collaborator Author

spawnia commented Jan 23, 2021

@vladar done 🚢

vladar
vladar approved these changes Jan 23, 2021
View reviewed changes
Copy link
Member

@vladar vladar left a comment

Choose a reason for hiding this comment

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

:shipit:

@vladar vladar merged commit f097de2 into webonyx:master Jan 23, 2021
@spawnia spawnia deleted the reify-ast-node-types branch January 23, 2021 12:41
@shmax
Copy link
Contributor

shmax commented Jan 30, 2021

@spawnia @vladar Great work, guys. 👍 By the way, sorry I haven't been very active lately; work has gotten a bit hectic, and my Big Project using graphql is basically done, but please don't hesitate to ping me if you would like me to look at any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simPod simPod simPod approved these changes

@vladar vladar vladar approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@spawnia @coveralls @vladar @shmax @simPod