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

Loader error message refactor #1439

Merged
merged 4 commits into from
May 28, 2020
Merged

Loader error message refactor #1439

merged 4 commits into from
May 28, 2020

Conversation

mstoykov
Copy link
Collaborator

@mstoykov mstoykov commented May 8, 2020

No description provided.

@codecov-io
Copy link

codecov-io commented May 8, 2020

Codecov Report

Merging #1439 into master will decrease coverage by 0.02%.
The diff coverage is 76.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1439      +/-   ##
==========================================
- Coverage   75.22%   75.20%   -0.03%     
==========================================
  Files         150      150              
  Lines       10911    10924      +13     
==========================================
+ Hits         8208     8215       +7     
- Misses       2238     2242       +4     
- Partials      465      467       +2     
Impacted Files Coverage Δ
loader/readsource.go 84.61% <50.00%> (-6.30%) ⬇️
loader/loader.go 83.22% <81.08%> (-1.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9586f2f...e109a96. Read the comment docs.

na--
na-- previously approved these changes May 8, 2020
@na--
Copy link
Member

na-- commented May 8, 2020

Please add an issue about the docs error pages in the k6-docs repo

@mstoykov
Copy link
Collaborator Author

mstoykov commented May 8, 2020

@na-- , sorry didn't see you've approved it when found that small issue :(

here is the k6-docs repo issue grafana/k6-docs#21

Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

As mentioned in Slack, I feel the messages are too verbose already and this adds to that, so there's no guarantee it will be read at all :)

But we can always structure and improve it later.

var noSchemeError noSchemeRemoteModuleResolutionError
if errors.As(err, &noSchemeError) {
// TODO maybe try to wrap the original error here as well, without butchering the message
return nil, fmt.Errorf(nothingWorkedLoadedMsg, noSchemeError.moduleSpecifier, noSchemeError.err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm using the latest golangci-lint version locally, and this line fails with goerr113 err113: do not define dynamic errors, use wrapped static errors instead, which is from a new linter in v1.26.0, and we should probably follow it. :)

I think it might be just a matter of using %w in the format string, and then we could also remove the Unwrap() method here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I understood it - I will need to define a new error type as I also have other parameters ... I also really wanted this to be a quick fix.
And this whole code probably needs additional error types, but again ... I wanted this to be 5 minutes fix so we stop getting all the module resolution problems.

This particular really long error message is only if you managed to not write the correct filename/url you want to execute, at which point, again, because of us doing schemeless remote module resolution we can have different errors, for things totally unrelated to what you meant to do.

If we didn't I could just write "file something.js wasn't found, if you are using docker ..." or "https://something.com returner error: ..." and be done with it...

@mstoykov mstoykov merged commit 479707f into master May 28, 2020
@mstoykov mstoykov deleted the loaderRefactor branch May 28, 2020 13:46
@mstoykov mstoykov added this to the v0.27.0 milestone Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants