feat: threw error if profile-create-new-if-missing used without profile #2073
Conversation
There was a problem hiding this comment.
@akhilpanchal Thanks for looking into quickly jumping on covering the followups related to #2064!!!
Follows a couple of review comments.
src/cmd/run.js
Outdated
| } else if (profileCreateIfMissing) { | ||
| throw new UsageError( | ||
| '--profile-create-if-missing should be paired ' + | ||
| 'with --firefox-profile or --chromium-profile' | ||
| ); |
There was a problem hiding this comment.
we could move this into the previous if block by doing something like:
if (profileCreateIfMissing) {
if (!profileDir) {
throw new UsageError("...);
}
// the rest of the if block from line 128 to line 133.
}
tests/unit/test-cmd/test.run.js
Outdated
| let actualError = null; | ||
| try { | ||
| await cmd.run({ profileCreateIfMissing: true }); | ||
| } catch (error) { | ||
| actualError = error; | ||
| } | ||
|
|
||
| assert.instanceOf(actualError, UsageError); | ||
| assert.match( | ||
| actualError && actualError.message, | ||
| /should be paired with --firefox-profile or --chromium-profile/ | ||
| ); |
There was a problem hiding this comment.
we should use await assert.isRejected(...) (provided by the chai-as-promised dev dependency and also used in some of the other tests, e.g. test.adb.js should be one of the test file already using this assertion functions).
…le option, improved tests
Thanks @rpl. I addressed your feedback in the latest commit. Thanks again for all your guidance and comments 😊 |
There was a problem hiding this comment.
@akhilpanchal good job, this looks great. I would just tweak the UsageError message a bit (my apologies I should have noticed it sooner and pointed it out in the previous review round) before merging this on master, would you mind to do that? (the tweak is described below).
src/cmd/run.js
Outdated
| if (profileCreateIfMissing) { | ||
| if (!profileDir) { | ||
| throw new UsageError( | ||
| '--profile-create-if-missing should be paired ' + |
There was a problem hiding this comment.
would you mind to replace should be paired with with requires? (mostly because I'm not really sure if paired is a common why to refer to that in english, "--profile-create-if-missing requires --firefox-profile or --chromium-profile" is shorter and should be clear enough).
Same change has to be applied to the assertion on the test side.
There was a problem hiding this comment.
@akhilpanchal good job, this looks great. I would just tweak the UsageError message a bit (my apologies I should have noticed it sooner and pointed it out in the previous review round) before merging this on master, would you mind to do that? (the tweak is described below).
No worries at all! Makes sense to me. It is shorter and clear.
I addressed it.
|
@akhilpanchal Thanks a lot! looks great and I'm going to merge it right now! 🎉 |
Follow up to #2064 based on @rpl's comment.
Throws error if
profile-create-new-if-missingoption used withoutfirefox-profileorchromium-profile.