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

config: add file path to error message when parsing fails #608

Merged
merged 2 commits into from
Apr 25, 2022

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Mar 2, 2022

Before:

undefined:5
}
^

SyntaxError: Unexpected token } in JSON at position 138
    at JSON.parse (<anonymous>)
    at exports.readJson (…/lib/file.js:38:17)
    at Object.exports.getConfig (…/lib/config.js:34:10)
    at Object.exports.getMergedConfig (…/lib/config.js:26:32)
    at Object.<anonymous> (…/components/git/metadata.js:8:44)
    at Module._compile (node:internal/modules/cjs/loader:1097:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1151:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:999:19)

Node.js v17.6.0

After:

…/lib/config.js:39
    throw err
    ^

Error: Unable to parse config file /home/~/.ncurc
    at Object.exports.getConfig (…/lib/config.js:37:17)
    at Object.exports.getMergedConfig (…/lib/config.js:26:32)
    ... 5 lines matching cause stack trace ...
    at Module.require (node:internal/modules/cjs/loader:999:19)
    at Object.require (node:internal/modules/cjs/helpers:102:18)
    at …/node_modules/require-directory/index.js:76:17 {
  cause: SyntaxError: Unexpected token } in JSON at position 138
      at JSON.parse (<anonymous>)
      at exports.readJson (…/lib/file.js:38:17)
      at Object.exports.getConfig (…/lib/config.js:35:12)
      at Object.exports.getMergedConfig (…/lib/config.js:26:32)
      at Object.<anonymous> (…/components/git/metadata.js:8:44)
      at Module._compile (node:internal/modules/cjs/loader:1097:14)
      at Object.Module._extensions..js (node:internal/modules/cjs/loader:1151:10)
      at Module.load (node:internal/modules/cjs/loader:975:32)
      at Function.Module._load (node:internal/modules/cjs/loader:822:12)
      at Module.require (node:internal/modules/cjs/loader:999:19)
}

Node.js v17.6.0

@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #608 (cb53cbb) into main (919ec3b) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #608      +/-   ##
==========================================
+ Coverage   84.10%   84.14%   +0.04%     
==========================================
  Files          37       37              
  Lines        4051     4069      +18     
==========================================
+ Hits         3407     3424      +17     
- Misses        644      645       +1     
Impacted Files Coverage Δ
lib/config.js 87.50% <100.00%> (+0.73%) ⬆️
lib/ci/ci_failure_parser.js 85.53% <0.00%> (-0.31%) ⬇️
lib/links.js 91.86% <0.00%> (-0.14%) ⬇️
lib/auth.js 87.60% <0.00%> (+1.88%) ⬆️

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 919ec3b...cb53cbb. Read the comment docs.

@Trott
Copy link
Member

Trott commented Mar 17, 2022

@aduh95 I think if you amend the commit message to start with fix: or feat: rather than config:, the linter will pass and this can land. (The Windows timeout is a known flaky and can be re-run.)

@targos
Copy link
Member

targos commented Mar 17, 2022

@aduh95 I think if you amend the commit message to start with fix: or feat: rather than config:, the linter will pass and this can land. (The Windows timeout is a known flaky and can be re-run.)

Yes. It will also be included in the changelog, and a release PR will be created (release-please only creates a PR if there's at least a fix, feat, or breaking change)

lib/config.js Outdated Show resolved Hide resolved
Co-authored-by: Michaël Zasso <targos@protonmail.com>
@targos targos merged commit 7c73862 into nodejs:main Apr 25, 2022
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

3 participants