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

fix: Improve error message for NonNull GQL types #1333

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #1192

Description

Improves the error message when attempting to declare a GQL schema with a NonNull field type.

@AndrewSisley AndrewSisley added feature New feature or request area/schema Related to the schema system action/no-benchmark Skips the action that runs the benchmark. labels Apr 11, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.5 milestone Apr 11, 2023
@AndrewSisley AndrewSisley requested a review from a team April 11, 2023 21:36
@AndrewSisley AndrewSisley self-assigned this Apr 11, 2023
@AndrewSisley AndrewSisley force-pushed the sisley/fix/I1192-not-null-err branch 2 times, most recently from 505c78f to bf2b2df Compare April 11, 2023 21:44
@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Merging #1333 (f2377d8) into develop (fe304d6) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1333      +/-   ##
===========================================
+ Coverage    70.51%   70.55%   +0.03%     
===========================================
  Files          184      184              
  Lines        17801    17803       +2     
===========================================
+ Hits         12553    12561       +8     
+ Misses        4289     4285       -4     
+ Partials       959      957       -2     
Impacted Files Coverage Δ
request/graphql/schema/errors.go 22.22% <ø> (ø)
request/graphql/schema/collection.go 84.92% <100.00%> (+3.19%) ⬆️

@AndrewSisley AndrewSisley force-pushed the sisley/fix/I1192-not-null-err branch from bf2b2df to f2377d8 Compare April 11, 2023 21:49
Comment on lines +39 to +41
// NonNull is the literal name of the GQL type, so we have to disable the linter
//nolint:revive
ErrNonNullNotSupported = errors.New("NonNull fields are not currently supported")
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Instead of suppressing with linter, can do this:

Suggested change
// NonNull is the literal name of the GQL type, so we have to disable the linter
//nolint:revive
ErrNonNullNotSupported = errors.New("NonNull fields are not currently supported")
// NonNull is the literal name of the GQL type, so we have to disable the linter
//nolint:revive
ErrNonNullNotSupported = errors.New("currently NonNull fields are not supported")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty against changing user-visible behaviour for the sake of linters, regardless of how minimal the change might be.

Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

LTGM. 1 Minor suggestion

@AndrewSisley AndrewSisley merged commit 1c80e10 into develop Apr 11, 2023
@AndrewSisley AndrewSisley deleted the sisley/fix/I1192-not-null-err branch April 11, 2023 22:05
shahzadlone pushed a commit that referenced this pull request Apr 13, 2023
* Add test adding schema with NonNull field

* Improve error message for NonNull GQL types
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
* Add test adding schema with NonNull field

* Improve error message for NonNull GQL types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/schema Related to the schema system feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when adding schema with non-null list type
2 participants