Skip to content

Conversation

kumar303
Copy link
Contributor

I was trying to track down the --pref option and I became confused until I realized it was getting aliased in program.js. I don't think this is the right place to alias it because it's not an obvious place to look. Also, it introduces an inconsistency since no other option is aliased like that. Most importantly, I'd like to keep the mapping of option names to top-level callback arguments one-to-one, whenever possible, for sanity.

@kumar303 kumar303 requested a review from rpl August 11, 2017 19:59
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7e779b0 on kumar303:clean-up-pref into e82fb5f on mozilla:master.

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.

@kumar303 r+

As briefly discussed over IRC, we should add a runtime check in cmd.run that throws a useful error message if the pref attribute is not of the expected type (mostly because when cmd.run will be used as a library and the caller will be not be covered by the flow type checks as the internal sources).

@kumar303 kumar303 merged commit ecbd47c into mozilla:master Aug 14, 2017
@kumar303 kumar303 deleted the clean-up-pref branch August 14, 2017 19:20
serendicoder pushed a commit to serendicoder/web-ext that referenced this pull request Aug 14, 2017
Rob--W added a commit to Rob--W/web-ext that referenced this pull request Aug 5, 2024
The customPrefs alias was introduced in mozilla#1039 as a direct alias,
but changed to a shallow copy in mozilla#2436 because the object was modified.
These changes have been dropped in mozilla#3136 but the swallow copy remained.

This patch completes the cleanup by reverting to a direct alias.
Rob--W added a commit that referenced this pull request Sep 20, 2024
The customPrefs alias was introduced in #1039 as a direct alias,
but changed to a shallow copy in #2436 because the object was modified.
These changes have been dropped in #3136 but the swallow copy remained.

This patch completes the cleanup by reverting to a direct alias.
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.

3 participants