Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use cwd-relative pathname to load config file #3829
Use cwd-relative pathname to load config file #3829
Changes from all commits
7d7154e
49c55cc
bcbcb6d
cf62435
e578813
5dcb8e3
e6a61d1
8bc0df6
e344846
a3b0489
00450dd
0af6421
24795ee
55ce117
9d46628
7251c18
b51af9d
4853993
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
hmm, does this cover both JSON and JS?
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.
so, as written, this is a breaking change, because the default becomes JS instead of JSON.
require()
will choke on a jsonc file.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.
require
will load both JS and JSON. Our documentation states JS is top priority. It unfortunately also states confusingly JSON is default; why?It can be argued that the latter refers to standard JSON, not the Mocha config file, so not breaking -- JSONC != JSON where this is concerned. This would only require documentation clarification.
For a real breaking change, Mocha should only allow comments in ".mocharc.jsonc". We should deprecate use of comments in ".mocharc.json" and follow the standard.
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.
"Priority" refers to the order in which configuration files are merged, not the order in which they are searched for. Sounds like that needs clarification.
IMO, this is unnecessarily hostile to the user; we don't lose anything by parsing
.json
files as JSONC, and we can't do that withrequire()
. Would appreciate other input on this. @craigtaub @cspotcode @juerba etc?So then my requested change is to revert the order so that the
else
block calls thejson
parser, and the previouselse if
block calls thejs
parser.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 objected to allowing comments to JSON when the yargs/config PR was first introduced, though I realized the potential need for comments in a config file. Now we have that via JSONC. But I fail to find any value in adding app-specific behavior to a standard file format that renders it unusable anywhere else. Perfect example right here -- we're potentially unable to load a JSON file via
require
because of our comment feature.And I wouldn't call it user hostile either. I think it would match user expectation for a JSON file to be able to pass JSONLint and to be
require
able.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 agree, we should follow standards whenever possible, which is user- and maintainer-friendly. Expectations are clearly defined for both sides.
So yes:
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.
That means we deprecate it instead of break it unless we want to cut v7.
Anybody who has a
.mocharc
(or specific config file lacking a file extension) containing JSON with comments will encounter an exception.If this is how we want to go, then I'll live with it, but I can't get behind releasing this as-is in a minor.
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 like to see the following config-related changes in v7.0:
require
-able.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.
If "breaking change" was previously unclear, I meant semver-major.
Deprecation sometime between now and then...
Simple change in meantime would be to remove the comments from "example/config/.mocharc.json", which can be done semver-patch.