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.

Sign up for GitHub

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

Print warning message for --opts argument #3352

Closed
wants to merge 5 commits into from
Closed

Print warning message for --opts argument #3352

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a348107
Print warning message
benedikt-roth Apr 25, 2018
8b200d3
Make linter happy
benedikt-roth Apr 25, 2018
2dbc42f
Print error instead of warning
benedikt-roth Apr 25, 2018
35508b2
Make linter happy
benedikt-roth Apr 25, 2018
f5cd0b5
Assign envr variable value as string
benedikt-roth Apr 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions bin/options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,18 @@ function getOptions() {
return;
}

const optsPath =
process.argv.indexOf('--opts') === -1
? 'test/mocha.opts'
: process.argv[process.argv.indexOf('--opts') + 1];
const optPathSpecified = process.argv.indexOf('--opts') > -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is nitpicky but can you please save the index? DRY and all that

const optsIndex = process.argv.indexOf('--opts');
const optsPathSpecified = optsIndex !== -1; // it will never be 0, so..
const optsPath = optsPathSpecified
  ? process.argv[optsIndex + 1]
  : 'test/mocha.opts';

const optsPath = optPathSpecified
? process.argv[process.argv.indexOf('--opts') + 1]
: 'test/mocha.opts';

if (optPathSpecified && !fs.existsSync(optsPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to just do it and recover instead of make this extra check.

So on L52 where we're catching the err, inspect it for err.code === 'ENOENT', then you can throw with your noSuchFileError, otherwise just rethrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

(in other words, avoid fs.existsSync)

const noSuchFileError = new Error(
`Specified options file does not exist: ${optsPath}`
);
noSuchFileError.code = 'ENOENT';
throw noSuchFileError;
}

try {
const opts = fs
Expand All @@ -44,5 +52,5 @@ function getOptions() {
// ignore
}

process.env.LOADED_MOCHA_OPTS = true;
process.env.LOADED_MOCHA_OPTS = 'true';
Copy link
Contributor

@boneskull boneskull Apr 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

please just change this to 1 for simplicity

OK, so, to be clear, this is good enough the way it is. It should be 1, but apparently some odd projects are depending on this for one reason or another, and 'true' isn't so painful that this needs changing. If this is causing other issues in Mocha, or creates otherwise significant technical debt, we're free to modify or remove it as we see fit.

Projects should not be depending on this variable, and should certainly not be depending on its value to be any particular thing other than set or unset.

Copy link
Contributor

@plroebuck plroebuck Apr 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Value needs to be 'true' to prevent breaking other npm packages unnecessarily.

Copy link
Contributor

Choose a reason for hiding this comment

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

What packages are using this? This is not documented and is not a "public" API (or an API at all, really).

Copy link
Author

Choose a reason for hiding this comment

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

I agree with @boneskull. LOADED_MOCHA_OPTS is intended for internal use only.
(Whereas this is not a good use case for an environment variable.)

}