add --filename option to specify a filename template #1900

escapewindow · 2020-05-05T01:55:52Z

Fixes #1335. Based off #1741 and its review comments.

Questions:

Does -n make sense as a single letter filename option? (n for name) I'm happy to add that or keep it as-is.
Is there a reason why we can't specify a directory path? I could see dying if the path specified doesn't exist, but I'm not sure why we can't write to a different directory.
The code, as written, changes the @ in the name to an underscore _ (minimal-example_web-ext-test-suite. Is that expected? Should we allow for people to name the file what they want, if they pass in a flag (or warn that their name doesn't follow certain conventions)?

JavaScript isn't my strong suit; please review accordingly :)

coveralls · 2020-05-05T02:19:50Z

Coverage remained the same at 100.0% when pulling c368ad6 on escapewindow:issue1335-2 into 7d31577 on mozilla:master.

escapewindow · 2020-05-05T17:45:10Z

I'm not sure if the audit-deps error is actionable. I may be misreading, but it looks like the security advisories are for older versions of the dependencies?

Am I on the right track @rpl ?

rpl

@escapewindow Thanks a lot for taking care of reviving the pull request for this enhancement!

I'm not sure if the audit-deps error is actionable. I may be misreading, but it looks like the security advisories are for older versions of the dependencies?

yeah, you are not misreading it. Don't worry about that, I'll take care of it in #1902, then you'll just need to rebase this once we merged that pull request and the audit-deps failure should go away.

Am I on the right track @rpl ?

This looks definitely on the right track, follows a round of review comments related to this version.

Questions:

Does -n make sense as a single letter filename option? (n for name) I'm happy to add that or keep it as-is.

👍 n isn't used for any other option, and so I think that I would be ok with it.

Is there a reason why we can't specify a directory path? I could see dying if the path specified doesn't exist, but I'm not sure why we can't write to a different directory.

well, we could probably allow it, but I have the feeling that we may then need to also check if any of the interpolated strings contains something that may be handled like a path to don't make that surprising (eg. "../../").

The developer can already customize the location of the artifacts dir with another one of the command line parameters, wouldn't that be enough for most of the use cases?

I would keep it simple in this PR and eventually re-evaluate that separately in a follow up if we agreed that there may be some reasonable use case that isn't already covered by customizing the artifacts dir location.

The code, as written, changes the @ in the name to an underscore _ (minimal-example_web-ext-test-suite. Is that expected? Should we allow for people to name the file what they want, if they pass in a flag (or warn that their name doesn't follow certain conventions)?

That should be due to safeFileName, I can't currently recall if there was any reason to not include @ into the safe characters, at a first glance it seems that @ should be actually fine, let me think about it, we can eventually add it to the regexp used inside safeFileName (we can as well do it in a follow up)

src/cmd/build.js

src/program.js

src/cmd/build.js

escapewindow · 2020-05-06T21:05:41Z

@escapewindow Thanks a lot for taking care of reviving the pull request for this enhancement!

I'm not sure if the audit-deps error is actionable. I may be misreading, but it looks like the security advisories are for older versions of the dependencies?

yeah, you are not misreading it. Don't worry about that, I'll take care of it in #1902, then you'll just need to rebase this once we merged that pull request and the audit-deps failure should go away.

Thanks! I rebased against master. Between that, and the commit lint, I had to rebase the entire PR. I'm hoping it's still easy to re-review?

Am I on the right track @rpl ?

This looks definitely on the right track, follows a round of review comments related to this version.

Thanks for your review! I think it's ready for re-review.

Questions:

Does -n make sense as a single letter filename option? (n for name) I'm happy to add that or keep it as-is.

👍 n isn't used for any other option, and so I think that I would be ok with it.

Added. I don't have a strong opinion here.

Is there a reason why we can't specify a directory path? I could see dying if the path specified doesn't exist, but I'm not sure why we can't write to a different directory.

well, we could probably allow it, but I have the feeling that we may then need to also check if any of the interpolated strings contains something that may be handled like a path to don't make that surprising (eg. "../../").

The developer can already customize the location of the artifacts dir with another one of the command line parameters, wouldn't that be enough for most of the use cases?

I would keep it simple in this PR and eventually re-evaluate that separately in a follow up if we agreed that there may be some reasonable use case that isn't already covered by customizing the artifacts dir location.

I think the -a option to specify the artifacts dir is sufficient. Thanks!

The code, as written, changes the @ in the name to an underscore _ (minimal-example_web-ext-test-suite. Is that expected? Should we allow for people to name the file what they want, if they pass in a flag (or warn that their name doesn't follow certain conventions)?

That should be due to safeFileName, I can't currently recall if there was any reason to not include @ into the safe characters, at a first glance it seems that @ should be actually fine, let me think about it, we can eventually add it to the regexp used inside safeFileName (we can as well do it in a follow up)

I'm still a bit concerned that the name isn't 100% deterministic -- we'll get different filenames if safeFileName munges it, or if it's localized. But in my real life tests, I was able to specify a name and directory, and got the expected output, so I think this is an overall improvement.

(Edit: By deterministic, I don't mean that the value is going to fluctuate between runs, but that the output might not be expected without digging into the extension's localization and the rules of safeFileName. I think if a predictable filename is the highest priority, we could do something like -n target.xpi, though, so we should be good.)

Thanks for your help!

rpl

Thanks! I rebased against master. Between that, and the commit lint, I had to rebase the entire PR. I'm hoping it's still easy to re-review?

This looks pretty good (and the diff is pretty clean and readable, thanks!)

There are just a couple of small additional issues that requires some additional changes (which should still be small enough), and a couple of optional nits, then this should be good to go.

src/program.js

src/cmd/build.js

src/program.js

src/cmd/build.js

tests/unit/test-cmd/test.build.js

rpl

This looks great to me. 👍 r+ \o/ 🎉

Thanks @escapewindow for reviving and then completing this pull request.

rpl · 2020-05-08T20:07:59Z

tests/unit/test-cmd/test.build.js

+        })
+          .then(makeSureItFails())
+          .catch((error) => {
+            log.info(error);


Nit, these long.info(error) are not really needed, I just noticed that we did already have one which got merge in a old patch, but not a big deal. We will have to rewrite all these tests into async functions at some point (to make them a bit more readable), and so don't worry about that.

rpl · 2020-05-08T20:12:56Z

tests/unit/test-cmd/test.build.js

@@ -86,6 +121,44 @@ describe('build', () => {
    );
  });

+  it('throws an error if the filename contains a path', () => {


Nit, it would probably good to have also one test that verifies that if an interpolated part of the template is a string that looks like a path then we still throw this kind of UsageError (which is actually the case with the version in this patch, it would be mostly an additional test to have more coverage in case of a refactoring), but we can deal with it in a follow up.

escapewindow · 2020-06-10T03:15:43Z

@rpl Is there a release planned anytime soon? We have a number of webextensions that would use this feature, if it were available. Thanks!

rpl · 2020-06-11T12:30:13Z

@rpl Is there a release planned anytime soon? We have a number of webextensions that would use this feature, if it were available. Thanks!

@escapewindow yeah, we should cut a new release and publish it on npm soon. Sorry for letting you wait.

rpl requested changes May 6, 2020

View reviewed changes

rpl requested changes May 8, 2020

View reviewed changes

src/program.js Outdated Show resolved Hide resolved

src/cmd/build.js Outdated Show resolved Hide resolved

src/program.js Outdated Show resolved Hide resolved

src/cmd/build.js Outdated Show resolved Hide resolved

tests/unit/test-cmd/test.build.js Outdated Show resolved Hide resolved

feat(--filename): add getPackageNameFromTemplate

c368ad6

rpl approved these changes May 8, 2020

View reviewed changes

rpl merged commit a58280d into mozilla:master May 11, 2020

escapewindow mentioned this pull request Jun 10, 2020

solution for predictable filenames mozilla-extensions/xpi-manifest#19

Closed

escapewindow mentioned this pull request Jun 16, 2020

Add login study mozilla-extensions/xpi-manifest#51

Merged

escapewindow mentioned this pull request Dec 10, 2020

Add firefox-quick-suggest-weather mozilla-extensions/xpi-manifest#83

Merged

Rob--W mentioned this pull request Jan 13, 2021

Cannot start extension on Android if name contains a slash #2119

Closed

escapewindow mentioned this pull request Feb 24, 2021

Continuous integration and end-to-end test scaffolding mozilla-extensions/firefox-translations#41

Merged

Rob--W mentioned this pull request Apr 13, 2021

fix: Sanitize manifest property values before interpolation #2222

Merged

Rob--W mentioned this pull request Feb 23, 2024

Add ability to build zip file without version string in file name #855

Closed

This was referenced Apr 20, 2024

pin the filename of the artifact file Robot-Inventor/shadowban-scanner#639

Closed

Update web-ext build options mozilla/extension-workshop#1834

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add --filename option to specify a filename template #1900

add --filename option to specify a filename template #1900

escapewindow commented May 5, 2020 •

edited

coveralls commented May 5, 2020 •

edited

escapewindow commented May 5, 2020 •

edited

rpl left a comment

escapewindow commented May 6, 2020 •

edited

rpl left a comment

rpl left a comment

rpl May 8, 2020

rpl May 8, 2020

escapewindow commented Jun 10, 2020

rpl commented Jun 11, 2020

add --filename option to specify a filename template #1900

add --filename option to specify a filename template #1900

Conversation

escapewindow commented May 5, 2020 • edited

coveralls commented May 5, 2020 • edited

escapewindow commented May 5, 2020 • edited

rpl left a comment

Choose a reason for hiding this comment

escapewindow commented May 6, 2020 • edited

rpl left a comment

Choose a reason for hiding this comment

rpl left a comment

Choose a reason for hiding this comment

rpl May 8, 2020

Choose a reason for hiding this comment

rpl May 8, 2020

Choose a reason for hiding this comment

escapewindow commented Jun 10, 2020

rpl commented Jun 11, 2020

escapewindow commented May 5, 2020 •

edited

coveralls commented May 5, 2020 •

edited

escapewindow commented May 5, 2020 •

edited

escapewindow commented May 6, 2020 •

edited