-
Notifications
You must be signed in to change notification settings - Fork 0
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
Displaying a friendlier error for unknown fields found in configuration files #2262
Conversation
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.
Had a few questions which might take these changes in the direction that the team had previously discussed. (Sorry, prior to your time.)
See what you think about my comments and let me know what questions you might have.
@sagerb @mmarchetti Thanks for the feedback 🙇 I'll move some things around and re-request review when it is ready for a second look |
@sagerb @mmarchetti This PR is ready for another look 🙂 |
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.
Going to reach out and move this to a verbal conversation. Then will update.
…compile to succeed
77cfbe2
to
51fcc81
Compare
096c20d
to
9c8ba4a
Compare
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 hate leaving the technical debt on the table for the suggestion I asked about, but if its not a fast change for you, I'm OK with merging without the change (and creating an issue to do it after the release).
// TODO: It would be better to have the config package methods as a provider pattern instead of plain functions | ||
var configFromFile cfgFromFile = config.FromFile | ||
var configGetConfigPath cfgGetConfigPath = config.GetConfigPath | ||
|
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.
Is there a reason we need to wait to make the change you indicated as a TODO?
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 like leaving TODO's like this but in this case the suggested change would have an impact on many other places using the config
package. That would expand this PR changes and also delay it's landing.
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 just had two questions pop up.
internal/services/api/api_errors.go
Outdated
} | ||
|
||
func (apierr *APIErrorInvalidTOMLFileDetails) JSONResponse(w http.ResponseWriter) { | ||
w.Header().Set("content-type", "application/json") |
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.
Saw the creation of JsonResult
and noticed it wasn't used in this file. Just curious if that was intentional. It looks like JsonResult
is only used once - should we consolidate into using it or leave this type of writer method calling?
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.
Got it, yes this can be confusing. There is a small difference between them though, but it can be handled better.
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.
Fantastic work on this file. Great exports, great helpers 💯
…nd rules. Include a resolution for rollup/parseAst
@dotNomad pushed a couple commits related to your questions. Things seem to be better now:
|
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.
Thanks for those additional changes @marcosnav! 🎉
Intent
In order to have more standardized errors handling, this PR introduces error types - to be expanded as we add more "known" errors.
Screenshot
Fixes #2157
Type of Change
Improvement
Approach
On the vscode extension UI code, changes made to
getSummaryStringFromError
allow to get a human consumable error message when the error contains a known JSON agent response - otherwise, a tracing-style message is returned to help diagnose.Automated Tests
Directions for Reviewers
abc = 123
).Checklist