Skip to content

fix: Added alias for watch-file option #2182

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

Merged
merged 5 commits into from
Mar 10, 2021

Conversation

ankushduacodes
Copy link
Contributor

Added alias for watch-file option and updated option description to reflect recent changes to the option

Continuation of #2125. Opening this PR in response to the discussion in extension-workshop PR #950 C3

@rpl Are there any tests to check if the alias does work as expected?

…ect recent changes related to supporting multiple files to watch mozilla#2125
@ankushduacodes
Copy link
Contributor Author

For some reason, I am not able to see open ci job that failed (sending back a blank screen), Will try again soon to see what the issue is.

Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

@ankushduacodes thanks a lot for creating a PR for the alias, just in time for including it into the new version.

About the tests: this kind of tests can be covered with a functional test, e.g. we may be able to cover it with just a couple of small changes to tests/functional/test.cli.run.js.

About the CI job that did fail:
oddly I got a white page when I tried to follow the link for the failure, but I got to the failure logs by navigating to it from the circleci homepage.
Anyway, the failure is only related to the commit message not following the convention:

There is only one commit in this pull request, we are going to check the single commit message...
Failures during changelog linting the pull request:

⧗   input: added alias for watch-file opt and updated option description to reflect recent changes related to supporting multiple files to watch #2125
✖   subject may not be empty [subject-empty]
✖   type may not be empty [type-empty]
⚠   header must not be longer than 72 characters, current length is 139 [header-max-length]

✖   found 2 problems, 1 warnings
ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint

Don't panic! If the CI job is failing here, please take a look at

 - https://github.com/mozilla/web-ext/blob/master/CONTRIBUTING.md#writing-commit-messages

Updating the commit message to follow the convention will fix that linting failure.

btw, I think in this case the conventional prefix should be fix: ... or chore: ....

src/program.js Outdated
alias: ['watch-files'],
describe: 'Reload the extension only when the content of these' +
' files change. This is useful if you use a custom' +
' build process for your extension or if you want web-ext' +
Copy link
Member

Choose a reason for hiding this comment

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

Using a "custom build process" is the most common reason for being interested into watching a specific file (or set of files) for changes, and that didn't change with the addition of the alias.
I think that in the end we may just leave the description as is.

Copy link
Contributor Author

@ankushduacodes ankushduacodes Mar 5, 2021

Choose a reason for hiding this comment

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

Got it. Going back...

@ankushduacodes ankushduacodes changed the title docs: Added alias for watch-file option and updated option description fix: Added alias for watch-file option and updated option description Mar 5, 2021
@ankushduacodes ankushduacodes changed the title fix: Added alias for watch-file option and updated option description fix: Added alias for watch-file option Mar 5, 2021
@ankushduacodes
Copy link
Contributor Author

ankushduacodes commented Mar 5, 2021

@rpl I added a test that does two things at the same time:

  1. Check for alias's working
  2. Providing multiple files in one option (Although yargs treat --watch-file file1 --watch-file file2 and --watch-file file1 file2 the same), I added this test because I had added same kind of test for watch-ignored as well.

Please let me know if any changes are required :)

@ankushduacodes ankushduacodes requested a review from rpl March 5, 2021 20:59
Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

lgtm

Thanks @ankushduacodes

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.

2 participants