Displaying a friendlier error for unknown fields found in configuration files #2262

marcosnav · 2024-09-05T20:49:04Z

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

Updated backend tests as needed.
Added new UI tests for the new code. (Run locally and will add something to test on CI too)

Directions for Reviewers

Edit the active configuration and add an invalid entry (such as abc = 123).
Save the file.
Notice the error displayed in the notification.

Checklist

I have updated CHANGELOG.md to cover notable changes.

…n unexpected field

…unexpected field

.github/CODEOWNERS

sagerb

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.

extensions/vscode/src/utils/errors.ts

extensions/vscode/src/views/homeView.ts

internal/services/api/api_helpers.go

internal/services/api/get_config_files.go

internal/util/toml.go

marcosnav · 2024-09-06T19:06:29Z

@sagerb @mmarchetti Thanks for the feedback 🙇 I'll move some things around and re-request review when it is ready for a second look

marcosnav · 2024-09-10T16:44:21Z

@sagerb @mmarchetti This PR is ready for another look 🙂

sagerb

Going to reach out and move this to a verbal conversation. Then will update.

internal/publish/publish.go

extensions/vscode/src/utils/errors.ts

…compile to succeed

sagerb

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).

sagerb · 2024-09-18T17:00:13Z

internal/services/api/get_config_files.go

+// 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
+


Is there a reason we need to wait to make the change you indicated as a TODO?

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.

dotNomad

I just had two questions pop up.

extensions/vscode/tsconfig.json

dotNomad · 2024-09-18T22:58:38Z

internal/services/api/api_errors.go

+}
+
+func (apierr *APIErrorInvalidTOMLFileDetails) JSONResponse(w http.ResponseWriter) {
+	w.Header().Set("content-type", "application/json")


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?

Got it, yes this can be confusing. There is a small difference between them though, but it can be handled better.

dotNomad · 2024-09-18T23:19:53Z

extensions/vscode/src/utils/errorTypes.ts

Fantastic work on this file. Great exports, great helpers 💯

…nd rules. Include a resolution for rollup/parseAst

marcosnav · 2024-09-19T17:43:01Z

@dotNomad pushed a couple commits related to your questions. Things seem to be better now:

test files evaluated by TS compiler
Adjusted JsonResult to receive status and be used thoroughly

dotNomad

Thanks for those additional changes @marcosnav! 🎉

marcosnav added 3 commits September 5, 2024 14:37

backend work to send a friendlier error when configuration file has a…

1a54be1

…n unexpected field

UI work to display a friendlier error when configuration file has an …

3f70cc0

…unexpected field

moving a couple humans as CODEOWNERS

ee0028b

marcosnav requested review from tdstein, dotNomad, kgartland-rstudio, mmarchetti and sagerb as code owners September 5, 2024 20:49

marcosnav self-assigned this Sep 5, 2024

marcosnav commented Sep 5, 2024

View reviewed changes

.github/CODEOWNERS Outdated Show resolved Hide resolved

Remove testing instance not used

8030d07

sagerb requested changes Sep 6, 2024

View reviewed changes

marcosnav added 2 commits September 10, 2024 10:06

PR feedback: Standardize error messaging

7530db9

Merge branch 'main' into config-err-friendly

7995423

sagerb reviewed Sep 12, 2024

View reviewed changes

internal/publish/publish.go Outdated Show resolved Hide resolved

extensions/vscode/src/utils/errors.ts Outdated Show resolved Hide resolved

marcosnav added 6 commits September 17, 2024 13:43

Agent changes to Api errors

9093eb6

Error handling types, vscode. Adding some tests too.

3367d2d

Fix test for windows paths matching

d4e59a5

Merge branch 'main' into config-err-friendly

bbc5d42

Clear up unused spies

ac492c6

Exclude test files within vscode tsconfig, which were preventing the …

51fcc81

…compile to succeed

marcosnav force-pushed the config-err-friendly branch from 77cfbe2 to 51fcc81 Compare September 18, 2024 15:50

Include unit tests for src dir to be run in CI

9c8ba4a

marcosnav force-pushed the config-err-friendly branch from 096c20d to 9c8ba4a Compare September 18, 2024 17:09

Merge branch 'main' into config-err-friendly

910e95b

sagerb approved these changes Sep 18, 2024

View reviewed changes

dotNomad approved these changes Sep 18, 2024

View reviewed changes

JsonResult usage

ae6269b

Do not exclude test files from tsconfig, allowing for type checking a…

81ce016

…nd rules. Include a resolution for rollup/parseAst

dotNomad approved these changes Sep 19, 2024

View reviewed changes

marcosnav merged commit 0b783b9 into main Sep 19, 2024
13 checks passed

marcosnav deleted the config-err-friendly branch September 19, 2024 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Displaying a friendlier error for unknown fields found in configuration files #2262

Displaying a friendlier error for unknown fields found in configuration files #2262

marcosnav commented Sep 5, 2024 •

edited

Loading

sagerb left a comment

marcosnav commented Sep 6, 2024

marcosnav commented Sep 10, 2024

sagerb left a comment

sagerb left a comment

sagerb Sep 18, 2024

marcosnav Sep 18, 2024

dotNomad left a comment

dotNomad Sep 18, 2024

marcosnav Sep 19, 2024

dotNomad Sep 18, 2024

marcosnav commented Sep 19, 2024

dotNomad left a comment

Displaying a friendlier error for unknown fields found in configuration files #2262

Displaying a friendlier error for unknown fields found in configuration files #2262

Conversation

marcosnav commented Sep 5, 2024 • edited Loading

Intent

Type of Change

Approach

Automated Tests

Directions for Reviewers

Checklist

sagerb left a comment

Choose a reason for hiding this comment

marcosnav commented Sep 6, 2024

marcosnav commented Sep 10, 2024

sagerb left a comment

Choose a reason for hiding this comment

sagerb left a comment

Choose a reason for hiding this comment

sagerb Sep 18, 2024

Choose a reason for hiding this comment

marcosnav Sep 18, 2024

Choose a reason for hiding this comment

dotNomad left a comment

Choose a reason for hiding this comment

dotNomad Sep 18, 2024

Choose a reason for hiding this comment

marcosnav Sep 19, 2024

Choose a reason for hiding this comment

dotNomad Sep 18, 2024

Choose a reason for hiding this comment

marcosnav commented Sep 19, 2024

dotNomad left a comment

Choose a reason for hiding this comment

marcosnav commented Sep 5, 2024 •

edited

Loading