Skip to content

Add documentation for --watch-file - #910

Merged
muffinresearch merged 3 commits into
mozilla:masterfrom
Dexterp37:patch-1
Mar 17, 2021
Merged

Add documentation for --watch-file#910
muffinresearch merged 3 commits into
mozilla:masterfrom
Dexterp37:patch-1

Conversation

@Dexterp37

@Dexterp37 Dexterp37 commented Feb 9, 2021

Copy link
Copy Markdown
Contributor

Fixes #872

This PR adds documentation for the --watch-file option (that was missing).

The behaviour for this option changed in mozilla/web-ext#2125.

This PR adds documentation for the `--watch-file` option (that was missing). The behaviour for this option changed in mozilla/web-ext#2125.
@ankushduacodes

Copy link
Copy Markdown
Contributor

Heads up for the linked issue #872.

Just wanted to make a note of it so I can close it when this PR lands :)

@ankushduacodes

ankushduacodes commented Feb 10, 2021

Copy link
Copy Markdown
Contributor

Question:

Shouldn't the --watch-file option now be called --watch-files instead to be more consistent with options like --ignore-files?

Suggestion:

--watch-file docs on web-ext run --help may also need some wording change to reflect the changes better.
https://github.com/mozilla/web-ext/blob/be8b8d3b8b967e88035826deb8141edd41c41b27/src/program.js#L619-L625

I apologise for not catching these while #2125 was still under development. Just got around to it :-)

cc: @rpl @Dexterp37

@rpl

rpl commented Feb 10, 2021

Copy link
Copy Markdown
Member

Question:

Shouldn't the --watch-file option now be called --watch-files instead to be more consistent with options like --ignore-files?

well, to be fair there are a number of other type: 'array' cli options that are also not plural (e.g. --target, --watch-ignored, --pref, --start-url).
In the end an option of type array doesn't have to be used for more than one value or more than one time, it is mainly "allowed to be used more than once" (also for yargs --watch-file file1 file2 and --watch-file file1 --watch-file file2 are equivalent and both valid), and so my personal opinion is that renaming the option isn't mandatory (especially given that the option did already exist before and the last change does just allow it to be used for more then just one watch file)

Alternatively we may use the same kind of approach we use for the --arg / --args option, and allow the users to use --watch-files as an alias:
https://github.com/mozilla/web-ext/blob/be8b8d3b8b967e88035826deb8141edd41c41b27/src/program.js#L664-L669

Still not strictly mandatory, but I would definitely be ok to make that change before release the new web-ext version.

@ankushduacodes This doesn't mean that your point wasn't a good one to raise, I actually appreciate you chiming in and point that out!

Suggestion:

--watch-file docs on web-ext run --help may also need some wording change to reflect the changes better.
https://github.com/mozilla/web-ext/blob/be8b8d3b8b967e88035826deb8141edd41c41b27/src/program.js#L619-L625

This sounds reasonable, we can still tweak the description in a separate PR (and maybe also add the watch-files alias as well) before we are going to release web-ext new version.

I apologise for not catching these while #2125 was still under development. Just got around to it :-)

No need to apologies, and your timing is actually still pretty great, we do have time to follow up before this change is being released to the users.

@rpl rpl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just noticed that I didn't added the nit comment here after we merged that change on the web-ext side.

It is also totally fine to merge this as is and then add the alias in a separate PR, and so I'm also marking this version as approved.


Environment variable: `$WEB_EXT_START_URL`

#### `--watch-file`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit, --watch-file, --watch-files (because we added the alias in web-ext v.6.0.0, mozilla/web-ext#2182)

@muffinresearch
muffinresearch merged commit 6700c42 into mozilla:master Mar 17, 2021
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.

Missing Documentation for --watch-file command line option in extension workshop

4 participants