-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Print warning message for --opts
argument
#3352
Conversation
When specified options file does not exist
--opts
argument
Like the idea and most of the coding implementation. But issue #2576 was for it to throw any error and die, rather than print warning and continue with defaults... Depending on how/where it is run, quite possible no one would notice the warning. /* bin/options.js#L33 - don't continue, die! */
var noSuchFileError = new Error(`no such options file: ${optsPath}`);
noSuchFileError.code = 'ENOENT';
throw noSuchFileError; |
Migrated to #3354 |
Went for the less invasive variant in the beginning, but I would also prefer to make the script fail. The code was changed to throw an error instead of a warning in 2dbc42f. Need some more feedback on this change. If it proves to be the way to go, I will add some tests. |
I agree on that value assignment (2). Fixed it in f5cd0b5. About the use of the env variable in general - It was introduced to fix #1980. Its purpose is not to try to process the options twice - once from the It looks like it was a quick fix and there might be a nicer way than to store it in a global env variable. Fixing that is the scope of a separate PR. I want to keep the changes rather small here. |
Benedikt, was not trying to imply my post-review notes were to be part of your issue changes. 1) and 3) probably should have another issue opened. My apologies if that was unclear due to my adding those notes. |
It's all right, @plroebuck. It is good to talk about it since we are already here 🙂 |
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.
Thanks! I have some comments was hoping you could address.
This is a breaking change, but it's a good one.
@@ -44,5 +52,5 @@ function getOptions() { | |||
// ignore | |||
} | |||
|
|||
process.env.LOADED_MOCHA_OPTS = true; | |||
process.env.LOADED_MOCHA_OPTS = 'true'; |
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.
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.
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.
Value needs to be 'true' to prevent breaking other npm packages unnecessarily.
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.
What packages are using this? This is not documented and is not a "public" API (or an API at all, really).
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 with @boneskull. LOADED_MOCHA_OPTS
is intended for internal use only.
(Whereas this is not a good use case for an environment variable.)
process.argv.indexOf('--opts') === -1 | ||
? 'test/mocha.opts' | ||
: process.argv[process.argv.indexOf('--opts') + 1]; | ||
const optPathSpecified = process.argv.indexOf('--opts') > -1; |
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.
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';
? process.argv[process.argv.indexOf('--opts') + 1] | ||
: 'test/mocha.opts'; | ||
|
||
if (optPathSpecified && !fs.existsSync(optsPath)) { |
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.
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.
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.
(in other words, avoid fs.existsSync
)
4547268
to
7613521
Compare
(Am on vocation right now. Gonna come back to this in about a week.)
…On Mon 30. Apr 2018 at 13:17, Christopher Hiller ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In bin/options.js
<#3352 (comment)>:
> @@ -44,5 +52,5 @@ function getOptions() {
// ignore
}
- process.env.LOADED_MOCHA_OPTS = true;
+ process.env.LOADED_MOCHA_OPTS = 'true';
What packages are using this? This is not documented and is not a "public"
API (or an API at all, really).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3352 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAOVWExt3wgtn21n5RfWdocU4FLtQebjks5tt0ccgaJpZM4TjID0>
.
|
Closing this one in favour of #3381. |
When specified options file does not exist
(Tackling #2576)
Requirements
Description of the Change
Alternate Designs
Why should this be in core?
Benefits
Less headache when your options file is not being loaded.
Possible Drawbacks
Applicable issues