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

Spanned arguments #1206

Closed
wants to merge 3 commits into from

Conversation

audunhalland
Copy link
Contributor

This change improves diagnostics when performing manual/domain-specific post-validation of arguments using LookAheadArguments. In the case of an error, the produced error message may be able to refer to the exact position in the problematic GraphQL arguments.

I figure this breaking change can be seriously considered, as Juniper looks set for a breaking release anyway.

@audunhalland
Copy link
Contributor Author

audunhalland commented Nov 3, 2023

A change that could be done to improve this is to somehow borrow the (start, end) tuple in LookAheadArguments instead of copying it. But the problem is that start and end are two independent fields of Spanning<T>, and those two fields can't be borrowed together without also borrowing the item: T, resulting in a leaky abstraction.

Edit: I propose a redesigned Spanning like this:

struct Spanning<T> {
    pub item: T,
    pub span: Span
}

struct Span {
    pub start: SourcePosition,
    pub end: SourcePosition,
}

@tyranron tyranron added enhancement Improvement of existing features or bugfix semver::breaking Breaking change in terms of SemVer k::api Related to API (application interface) labels Nov 7, 2023
@tyranron
Copy link
Member

tyranron commented Nov 7, 2023

@audunhalland could you provide some meaningful examples where do you want this?

Copy link
Member

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@audunhalland could you provide some meaningful examples where do you want this?

Giving it a little bit more thoughts, I now better understand how this may be useful, and overall okay with this course of changes.

Edit: I propose a redesigned Spanning like this:

struct Spanning<T> {
    pub item: T,
    pub span: Span
}

struct Span {
    pub start: SourcePosition,
    pub end: SourcePosition,
}

Yup, that would be better.

@audunhalland
Copy link
Contributor Author

@tyranron thank you for the review! I think the Spanning redesign should be a separate PR and I'll instead rebase this one.

@audunhalland
Copy link
Contributor Author

audunhalland commented Nov 8, 2023

That PR is up at #1207

@audunhalland
Copy link
Contributor Author

I'll close this PR and open a new one on top of #1208 later.

@tyranron tyranron added this to the 0.16.0 milestone Nov 9, 2023
@tyranron tyranron added wontfix This will not be worked on duplicate This issue or pull request already exists area::parsing Related to GraphQL parsing labels Nov 9, 2023
@tyranron tyranron mentioned this pull request Nov 9, 2023
tyranron pushed a commit that referenced this pull request Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area::parsing Related to GraphQL parsing duplicate This issue or pull request already exists enhancement Improvement of existing features or bugfix k::api Related to API (application interface) semver::breaking Breaking change in terms of SemVer wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants