-
Notifications
You must be signed in to change notification settings - Fork 44
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
refactor: Extract query schema errors to dedicated file #1037
Conversation
e3cf82b
to
6a218eb
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1037 +/- ##
===========================================
- Coverage 57.78% 57.75% -0.03%
===========================================
Files 173 174 +1
Lines 19495 19521 +26
===========================================
+ Hits 11265 11275 +10
- Misses 7235 7251 +16
Partials 995 995
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a note about the "generic" error type from the client
package.
errors.NewKV("Expected type", "*gql.InputObject"), | ||
errors.NewKV("Actual type", fmt.Sprintf("%T", operatorType)), | ||
) | ||
return nil, client.NewErrUnexpectedType[*gql.InputObject]("operatorType", operatorType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I know the use of generics for this particular err type is from before, but the more I see it the more I don't like it :/
I could just as easily be client.NewErrUnexpectedType("operatortype", &gql.InputObject{}, operatorType)
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) Why would you instantiate a whole object (at runtime) just for a type declaration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, its the error path, so allocation/perf isn't the highest priority, besides you allocate it anyway within the func.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to leave generics when we know we really want to maintain type safety. I don't see that as a requirement in this particular use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generics are not just for type safety :)
My 'Why would you...' comment isn't really just about performance. An object is a 'thing', and that thing lives in the runtime space and is mutable/variable. A type declaration is a compile-time constant and never changes. By defining as much as possible at compile time you can reduce the amount of variation in a system.
That said, this also smells like personal preference, and scope creep as if I was to change this here I'd have to expand this PR to cover every line that touches this function. I also don't think it would make sense to slow the merge of this PR for this even if we were in agreement that it should change in the very near future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really see why the use of generics here wouldn't be appropriate. I suggest we take this conversation to a Discord thread as it's out of scope of this PR. John, if you really think we shouldn't do that, I would love to have your thoughts explained in the Discord thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed on scope, was raised here its was the N'th time it poked me. Doesn't need to block the merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Create issue to change this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
6a218eb
to
f5003b4
Compare
Relevant issue(s)
Part of #257
Description
Extracts query schema errors to dedicated files (includes mapper). Continuation of work done at the end of last year whilst waiting on other discussions to progress.