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

Visitor Fixes and Editing Support #134

Merged

Conversation

NeedleInAJayStack
Copy link
Member

This adds editing support to the AST Visitor system. In addition, it also:

  • Adds all graphql-js visitor tests cases, both for editing and for normal visitor operation
  • Fixes a bug where the Visitor would not visit all Directive, ListType, or ListValue properties
  • Fixes a bug where Visitor path would be generated incorrectly
  • Improves Visitor null safety

@NeedleInAJayStack
Copy link
Member Author

@paulofaria Looks like the any keyword isn't supported in Swift 5.5. What do you think about dropping support for that version?

@paulofaria
Copy link
Member

Hm, I'm not sure we can, based on the SSWG guidelines? Can't we just remove the keyword so it's backwards compatible?

@NeedleInAJayStack
Copy link
Member Author

Oh weird - I added any because I thought I saw error messages, but I wasn't able to recreate them. I've removed the any keywords.

Maybe not relevant anymore, but I don't think SSWG requires extensive backsupport - New NIO versions only back-support to Swift 5.7: https://github.com/apple/swift-nio/#swiftnio-2

paulofaria
paulofaria previously approved these changes Nov 10, 2023
Copy link
Member

@paulofaria paulofaria left a comment

Choose a reason for hiding this comment

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

Beautiful!

@NeedleInAJayStack
Copy link
Member Author

Thanks @paulofaria. I just made one last small change to back-support the public Node Protocol API, making this a minor change instead of major.

Mind approving again?

@NeedleInAJayStack NeedleInAJayStack merged commit db0d3fe into GraphQLSwift:main Nov 11, 2023
7 checks passed
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.

2 participants