Skip to content
This repository has been archived by the owner on Oct 14, 2020. It is now read-only.

[v5] Npm run format #119

Merged
merged 5 commits into from
Apr 3, 2018
Merged

[v5] Npm run format #119

merged 5 commits into from
Apr 3, 2018

Conversation

motin
Copy link
Contributor

@motin motin commented Mar 10, 2018

Targeting develop branch, for v5.

This is an updated version of PR #109 (see that PR for general discussion)

It includes the commits for #107 and #118 and should thus not be merged before those are merged (to the develop branch) and this PR is rebased upon the latest develop branch.

@motin motin closed this Mar 23, 2018
@motin motin deleted the npm-run-format branch March 23, 2018 23:03
@motin motin restored the npm-run-format branch March 23, 2018 23:04
@motin motin reopened this Mar 23, 2018
Copy link
Collaborator

@biancadanforth biancadanforth left a comment

Choose a reason for hiding this comment

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

Left a couple minor comments; one's fixed by rebasing against the current develop branch, and the only real weirdness was a comma after an argument when it's the only argument for a function. Possibly look into the prettier rule for that and update?

.eslintrc.js Outdated
semi: ["error", "always"],
"require-jsdoc": "warn",
"valid-jsdoc": "warn",
"max-len": ["error", 80],
Copy link
Collaborator

Choose a reason for hiding this comment

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

In PR #107 I changed "max-len" to "warn" (just earlier today; hadn't looked at that PR in 5 months); I figure that's a rule some people have very strong feelings for/against. Rebasing against the current develop with PR #107 merged in should fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This rule is anyways removed in #120 since it follows the suggested config from mozilla/shield-studies-addon-template#61 as a base.

@@ -58,30 +58,44 @@ module.exports.promiseSetupDriver = async() => {
.forBrowser("firefox")
.setFirefoxOptions(options);

const binaryLocation = await promiseActualBinary(process.env.FIREFOX_BINARY || "firefox");
const binaryLocation = await promiseActualBinary(
process.env.FIREFOX_BINARY || "firefox",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not expect a comma here after the first arg. Seems like Prettier is getting confused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but leaving as is since changing this line here requires major rebasing and conflict handling in the commits of #120 so I suggest that this is merged as is and if it is still a problem after #120, it should be cleaned up in the develop branch later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this sound related? prettier/prettier#2788, and if so, should we change https://github.com/mozilla/shield-studies-addon-utils/pull/119/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R52 and we should maybe change --trailing-comma=all to --trailing-comma=es5?

Trailing Commas

Print trailing commas wherever possible when multi-line. (A single-line array,
for example, never gets trailing commas.)

Valid options:

  • "none" - No trailing commas.
  • "es5" - Trailing commas where valid in ES5 (objects, arrays, etc.)
  • "all" - Trailing commas wherever possible (including function arguments). This requires node 8 or a transform.

@motin
Copy link
Contributor Author

motin commented Apr 3, 2018

Rebased on current develop branch (which now includes #107) and the updated #118. Please merge #118 and I can rebase this again upon the develop branch. OR simply merge #120 since it includes this pr and #118.

@gregglind gregglind merged commit af564c7 into mozilla:develop Apr 3, 2018
@motin motin deleted the npm-run-format branch April 4, 2018 19:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants