Fwd: Upcoming changes to our JS coding style
Begin forwarded message:
From: Victor Porof vporof@mozilla.com
Date: 15. June 2019 at 7:27:41 AM CEST
To: firefox-dev@mozilla.org
Subject: Upcoming changes to our JS coding style
This email was also sent to dev-platform a few days ago, and a copy can be found here. In case there’s folks who aren’t subscribed to dev-platform, I’m forwarding it here on firefox-dev as well.
TL;DR
To improve developer productivity, we plan to automate JS formatting using Prettier, a broadly adopted tool specifically designed for this task and maintained by well known stakeholders. This should reduce the amount of time spent writing and reviewing JS code.
This choice was informed by explicit goals such as compatibility with third-party libraries (e.g. React), consistency, and predictability. The resulting integration includes automation (try, phabricator) as well as CLI (mach).
Changes have been tested and verified with a small part of the codebase before being rolled out to the entire tree.
Background
Mozilla is more invested than ever in using automated tools for checking and rewriting code. Various tools are already in use on CI, for example clang-format, checkstyle, flake8, as well as C/C++ static analysis. For JS, eslint is primarily used for ensuring code quality, however consistent and predictable styling was a non-goal until now.
As with C++, Mozilla has had a coding style for JS, but it’s not entirely enforced through tooling. In addition, the current style is only used in our projects; by contrast, Prettier is complete, well supported in tooling, and widely used by many other stakeholders.
We still have styling inconsistencies in our codebase. On top of that, we have been spending a lot of time talking about and adjusting whitespace. This wastes human time when writing, reviewing, and discussing code.
Details
We will build on the existing tooling foundation by applying coding style checks and automated rewrites throughout the authoring process (pre-commit, commit, review, landing).
We’re announcing the following changes to our coding style and its enforcement through our commit policy:
- We will migrate to the Prettier coding style to encourage more consistent code. We will be preserving all other aspects pertaining to code quality (linting) through eslint.
- We will check conformance with coding style rules upon review and landing, so that issues can be easily addressed or fixed through automation. The preference will be to enforce style issues as early as possible (before landing) to avoid late surprises.
- We will automatically enforce restrictions on formatting of whitespace (such as indentation and braces). For other stylistic issues (such as naming), no particular style is enforced, and consistency with surrounding code should take precedence.
These changes have been approved both by Firefox senior engineering leadership and the Firefox Technical Leadership Module.
We’ve already smoke-tested these changes, as well as fine tuned various aspects of the tooling with the DevTools and Activity Stream teams.
When?
A month ago, we pushed a patch to reformat and enable CLI and automation support for Prettier in a section of the tree (devtools/debugger). This first step was intended to be a smoke test in a controlled environment with a team of developers who have volunteered to help us test this change. We haven’t found any major surprises.
Last Friday we landed top-level support for Prettier, as a preliminary stop-gap for allowing folks to check out what ./mach lint --fix does for them, and manually whitelist sources in the meantime if needed.
Assuming that everything continues to go well, on Friday, July 5, we will push a patch to mozilla-central in order to rewrite the entire tree using Prettier.
We want to do this work right before a merge (when nightly version moves to beta) to limit the impact on the beta uplift process. To mitigate the impact on the developers who backport fixes to ESR, two weeks after the merge we will also reformat the ESR68 code base. A more detailed roadmap is available here.
Next week during Whistler, we also invite you to the “Using Prettier across Mozilla” lightning talk to discuss in person about this project.
How?
To reformat code locally after the landing on July 5:
$ ./mach lint <glob> --fix
Eslint integrations for most popular code editors already allow developers to easily format the code they’re working on at the time they’re making changes to it. Prettier leverages this infrastructure and is triggered automatically when running eslint --fix. If your code editor has an eslint plugin, enable auto-fix to trigger Prettier. We’ll share some tutorials about this soon.
Providing Feedback
This post is intended as an announcement, but we do welcome your feedback on this, both at a high level and at the technical level.
For high level feedback please reach out to vporof@mozilla.com. For technical feedback (e.g. bugs about the conversion process) please file bugs under bug prettier-format.
We understand that coding styles can be subjective. Many people would agree that having a consistent style across the code base is valuable, and that’s not where we are now. This change doesn’t mean that the coding style will remain forever this way; it gives us the opportunity to easily change our coding style and apply the change on the code base. It’s the first step to actually having consistency.
Thanks to the many who have helped out technically and with planning, including Dave Townsend, Mark Banner, Dan Mosedale, Kate Hudson, Ed Lee, Jason Laster, Sylvestre Ledru, Andi-Bogdan Postelnicu, Sebastian Hengst and Ritu Kothari.
Victor
FAQ:
- Can I see what it looks like?
To reformat code locally today, apply the patches in bug 1558517 and whitelist the respective sources in the top level .prettierignore file, then:
$ ./mach lint <glob> --fix
A formatted repository is available on Github: https://github.com/victorporof/gecko-dev/tree/prettier to visualise the changes.
- Can I ignore just a file?
If you have a good reason, yes. Just add it into .prettierignore with a comment explaining why it should be ignored.
- Can I ignore a whole directory?
If it’s third party code, yes! Just add it into thirdpartypaths.txt and mach will ignore it. Otherwise, you can also add a glob to .prettierignore.
- Why not continue using eslint?
To a great degree, we’ve moved closer towards a consistent codebase using eslint. However, eslint specializes in code quality checks, and not formatting.
For example, auto-fix is not available for most stylistic issues. Another problem pertains to output being inconsistent depending on the initial styling. Furthermore, depending on which rules are chosen, conflicts may also arise and successive runs can result in differently styled output. All of those issues are incompatible with our goals. To enforce a coding style, consistency and predictability are necessary, both of which are not possible with eslint.
We’ll continue having eslint coexisting with Prettier. Rules strictly pertaining to code quality will be preserved, and this task will continue to be in eslint's domain. Most styling-related rules will be removed in favour of separating this concern into Prettier’s domain. Both tools will be running in tandem.
- Why not something else?
Our choice of Prettier was informed by meticulously analyzing existing JS formatters, including eslint, jsbeautify and clang-format. We’ve tested reformatting mozilla-central using each one of them, measuring consistency (“is the output predictable regardless of source?”), integration effort (“can this fit into our CLI and build infrastructure?”), build/server time cost (“is this fast enough?”), capabilities (“can this parse our funny-looking JS or JSX?”) and also configurability (“can this tool suit our existing code quality checks?”).
In addition, every single rule for all of those tools was also individually tested against all of mozilla-central, measuring code churn. We’ve gathered a lot of info, and decided that a minimal configuration is the least impactful objectively: all other configuration rules have relatively equal churn across the codebase regardless of the settings used, so we defined those as subjective and settled for the defaults.
- Will this break blame on VCS?
As with the clang-format rollout for reformatting C++, we will tag the formatting changeset commit message with “skip-blame” so that Mercurial would automatically ignore the reformat changeset for blame operations.
- Will I have to rebase?
In order to mitigate the impact for pending changes, we will re-enable format-source and make it work with Prettier, to locally reformat the changes before the big patch is pulled. This will fix most of the conflict issues.
We will share more details about this later.
firefox-dev mailing list
firefox-dev@mozilla.org
https://mail.mozilla.org/listinfo/firefox-dev
Further to this, Prettier is currently disabled
https://hg.mozilla.org/comm-central/annotate/e73ec57351765daf1d9e629afc07e7dc54b07018/.eslintrc.js#l51
on comm-central. This will almost certainly change at some point but to
my knowledge there are no plans to do it immediately.
GL
On 15/06/19 23:14, Philipp Kewisch wrote:
Begin forwarded message:
From: Victor Porof <vporof@mozilla.com mailto:vporof@mozilla.com>
Date: 15. June 2019 at 7:27:41 AM CEST
To: firefox-dev@mozilla.org mailto:firefox-dev@mozilla.org
Subject: Upcoming changes to our JS coding style
This email was also sent to dev-platform a few days ago, and a copy
can be found here
https://docs.google.com/document/d/1UDERMflocqdszMGhhtxiVhaCTVOHo6jxLsGbt4BR9uw.
In case there’s folks who aren’t subscribed to dev-platform, I’m
forwarding it here on firefox-dev as well.
TL;DR
To improve developer productivity, we plan to automate JS formatting
using Prettier https://prettier.io/, a broadly adopted tool
specifically designed for this task and maintained by well known
stakeholders. This should reduce the amount of time spent writing and
reviewing JS code.
This choice was informed by explicit goals such as compatibility with
third-party libraries (e.g. React), consistency, and predictability.
The resulting integration includes automation (try, phabricator) as
well as CLI (mach).
Changes have been tested and verified with a small part of the
codebase before being rolled out to the entire tree.
Background
Mozilla is more invested than ever in using automated tools for
checking and rewriting code. Various tools are already in use on CI,
for example clang-format
https://clang.llvm.org/docs/ClangFormat.html, checkstyle
https://github.com/checkstyle/checkstyle, flake8
http://flake8.pycqa.org/en/latest/, as well as C/C++ static
analysis. For JS, eslint https://eslint.org/ is primarily used for
ensuring code quality, however consistent and predictable styling was
a non-goal until now.
As with C++, Mozilla has had a coding style for JS, but it’s not
entirely enforced through tooling. In addition, the current style is
only used in our projects; by contrast, Prettier is complete, well
supported https://www.npmjs.com/browse/depended/prettier in
tooling, and widely used https://prettier.io/en/users/ by many
other stakeholders.
We still have styling inconsistencies in our codebase. On top of
that, we have been spending a lot of time talking about and adjusting
whitespace. This wastes human time when writing, reviewing, and
discussing code.
Details
We will build on the existing tooling foundation by applying coding
style checks and automated rewrites throughout the authoring process
(pre-commit, commit, review, landing).
We’re announcing the following changes to our coding style and its
enforcement through our commit policy:
- We will migrate to the Prettier coding style to encourage more
consistent code. We will be preserving all other aspects pertaining
to code quality (linting) through eslint. - We will check conformance with coding style rules upon review and
landing, so that issues can be easily addressed or fixed through
automation. The preference will be to enforce style issues as early
as possible (before landing) to avoid late surprises. - We will automatically enforce restrictions on formatting of
whitespace (such as indentation and braces). For other stylistic
issues (such as naming), no particular style is enforced, and
consistency with surrounding code should take precedence.
These changes have been approved both by Firefox senior engineering
leadership and the Firefox Technical Leadership Module
https://wiki.mozilla.org/Modules/Firefox_Technical_Leadership.
We’ve already smoke-tested these changes, as well as fine tuned
various aspects of the tooling with the DevTools and Activity Stream
teams.
When?
A month ago, we pushed a patch
https://bugzilla.mozilla.org/show_bug.cgi?id=1551218 to reformat
and enable CLI and automation support for Prettier in a section of
the tree (devtools/debugger). This first step was intended to be a
smoke test in a controlled environment with a team of developers who
have volunteered to help us test this change. We haven’t found any
major surprises.
Last Friday we landed
https://bugzilla.mozilla.org/show_bug.cgi?id=1556013 top-level
support for Prettier, as a preliminary stop-gap for allowing folks to
check out what ./mach lint --fix does for them, and manually
whitelist sources in the meantime if needed.
Assuming that everything continues to go well, on Friday, July 5, we
will push a patch to mozilla-central in order to rewrite the entire
tree using Prettier.
We want to do this work right before a merge (when nightly version
moves to beta) to limit the impact on the beta uplift process. To
mitigate the impact on the developers who backport fixes to ESR, two
weeks after the merge we will also reformat the ESR68 code base. A
more detailed roadmap is available here
https://docs.google.com/document/d/1qV3aULyhulHsNHvnlbgAxeqlMGnpklUnxmpnY1OovXk/edit#.
Next week during Whistler, we also invite you to the “Using Prettier
across Mozilla” lightning talk to discuss in person about this project.
How?
To reformat code locally after the landing on July 5:
$ ./mach lint <glob> --fix
Eslint integrations for most popular code editors already allow
developers to easily format the code they’re working on at the time
they’re making changes to it. Prettier leverages this infrastructure
and is triggered automatically when running eslint --fix. If your
code editor has an eslint plugin, enable auto-fix to trigger
Prettier. We’ll share some tutorials about this soon.
Providing Feedback
This post is intended as an announcement, but we do welcome your
feedback on this, both at a high level and at the technical level.
For high level feedback please reach out to vporof@mozilla.com
mailto:vporof@mozilla.com. For technical feedback (e.g. bugs about
the conversion process) please file bugs under bug prettier-format
https://bugzilla.mozilla.org/show_bug.cgi?id=1558517.
We understand that coding styles can be subjective. Many people would
agree that having a consistent style across the code base is
valuable, and that’s not where we are now. This change doesn’t mean
that the coding style will remain forever this way; it gives us the
opportunity to easily change our coding style and apply the change on
the code base. It’s the first step to actually having consistency.
Thanks to the many who have helped out technically and with planning,
including Dave Townsend, Mark Banner, Dan Mosedale, Kate Hudson, Ed
Lee, Jason Laster, Sylvestre Ledru, Andi-Bogdan Postelnicu, Sebastian
Hengst and Ritu Kothari.
Victor
FAQ:
- Can I see what it looks like?
To reformat code locally today, apply the patches in bug 1558517
https://bugzilla.mozilla.org/show_bug.cgi?id=1558517 and whitelist
the respective sources in the top level .prettierignor
https://dxr.mozilla.org/mozilla-central/source/.prettierignoree
https://dxr.mozilla.org/mozilla-central/source/.prettierignore
file, then:
$ ./mach lint <glob> --fix
A formatted repository is available on Github:
https://github.com/victorporof/gecko-dev/tree/prettier to visualise
the changes.
- Can I ignore just a file?
If you have a good reason, yes. Just add it into .prettierignore
https://dxr.mozilla.org/mozilla-central/source/.prettierignore with
a comment explaining why it should be ignored.
- Can I ignore a whole directory?
If it’s third party code, yes! Just add it into thirdpartypaths.txt
https://dxr.mozilla.org/mozilla-central/source/tools/rewriting/ThirdPartyPaths.txt?q=thirdpartypaths.txt&redirect_type=direct
and mach will ignore it. Otherwise, you can also add a glob to
.prettierignore
https://dxr.mozilla.org/mozilla-central/source/.prettierignore.
- Why not continue using eslint?
To a great degree, we’ve moved closer towards a consistent codebase
using eslint. However, eslint specializes in code quality checks, and
not formatting.
For example, auto-fix is not available for most stylistic issues
https://eslint.org/docs/rules/#stylistic-issues. Another problem
pertains to output being inconsistent depending on the initial
styling. Furthermore, depending on which rules are chosen, conflicts
may also arise and successive runs can result in differently styled
output. All of those issues are incompatible with our goals. To
enforce a coding style, consistency and predictability are necessary,
both of which are not possible with eslint.
We’ll continue having eslint coexisting with Prettier. Rules strictly
pertaining to code quality will be preserved, and this task will
continue to be in eslint's domain. Most styling-related rules will be
removed in favour of separating this concern into Prettier’s domain.
Both tools will be running in tandem.
- Why not something else?
Our choice of Prettier was informed by meticulously analyzing
existing JS formatters, including eslint, jsbeautify and
clang-format. We’ve tested reformatting mozilla-central using each
one of them, measuring consistency (“is the output predictable
regardless of source?”), integration effort (“can this fit into our
CLI and build infrastructure?”), build/server time cost (“is this
fast enough?”), capabilities (“can this parse our funny-looking JS or
JSX?”) and also configurability (“can this tool suit our existing
code quality checks?”).
In addition, every single rule for all of those tools was also
individually tested against all of mozilla-central, measuring code
churn. We’ve gathered a lot of info, and decided that a minimal
configuration
https://dxr.mozilla.org/mozilla-central/source/.prettierrc is the
least impactful objectively: all other configuration rules have
relatively equal churn across the codebase regardless of the settings
used, so we defined those as subjective and settled for the defaults.
- Will this break blame on VCS?
As with the clang-format rollout for reformatting C++, we will tag
the formatting changeset commit message with “skip-blame” so that
Mercurial would automatically ignore the reformat changeset for blame
operations.
- Will I have to rebase?
In order to mitigate the impact for pending changes, we will
re-enable format-source
https://pypi.python.org/pypi/hg-formatsource and make it work with
Prettier, to locally reformat the changes before the big patch is
pulled. This will fix most of the conflict issues.
We will share more details about this later.
firefox-dev mailing list
firefox-dev@mozilla.org mailto:firefox-dev@mozilla.org
https://mail.mozilla.org/listinfo/firefox-dev
IMHO, this is great news for those working on Firefox. I'd really like
to see us do the same for Thunderbird.
I have found, when using Prettier for JS and when writing Scheme, that
it is so nice not to have to worry about formatting and keeping it
consistent, so much easier to just let the computer do that for you with
a keystroke or two.
-Paul
This is definitely something we need to follow through with in
comm-central too.
I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1572047 to
implement this change and running a formatting pass across the code base.
It should be a pretty good productivity boost as reviews no longer need
to care much about code formatting issues.
-Magnus
On 15-06-2019 14:14, Philipp Kewisch wrote:
Begin forwarded message:
From: Victor Porof <vporof@mozilla.com mailto:vporof@mozilla.com>
Date: 15. June 2019 at 7:27:41 AM CEST
To: firefox-dev@mozilla.org mailto:firefox-dev@mozilla.org
Subject: Upcoming changes to our JS coding style
This email was also sent to dev-platform a few days ago, and a copy
can be found here
https://docs.google.com/document/d/1UDERMflocqdszMGhhtxiVhaCTVOHo6jxLsGbt4BR9uw.
In case there’s folks who aren’t subscribed to dev-platform, I’m
forwarding it here on firefox-dev as well.
TL;DR
To improve developer productivity, we plan to automate JS formatting
using Prettier https://prettier.io/, a broadly adopted tool
specifically designed for this task and maintained by well known
stakeholders. This should reduce the amount of time spent writing and
reviewing JS code.
This choice was informed by explicit goals such as compatibility with
third-party libraries (e.g. React), consistency, and predictability.
The resulting integration includes automation (try, phabricator) as
well as CLI (mach).
Changes have been tested and verified with a small part of the
codebase before being rolled out to the entire tree.
Background
Mozilla is more invested than ever in using automated tools for
checking and rewriting code. Various tools are already in use on CI,
for example clang-format
https://clang.llvm.org/docs/ClangFormat.html, checkstyle
https://github.com/checkstyle/checkstyle, flake8
http://flake8.pycqa.org/en/latest/, as well as C/C++ static
analysis. For JS, eslint https://eslint.org/ is primarily used for
ensuring code quality, however consistent and predictable styling was
a non-goal until now.
As with C++, Mozilla has had a coding style for JS, but it’s not
entirely enforced through tooling. In addition, the current style is
only used in our projects; by contrast, Prettier is complete, well
supported https://www.npmjs.com/browse/depended/prettier in
tooling, and widely used https://prettier.io/en/users/ by many
other stakeholders.
We still have styling inconsistencies in our codebase. On top of
that, we have been spending a lot of time talking about and adjusting
whitespace. This wastes human time when writing, reviewing, and
discussing code.
Details
We will build on the existing tooling foundation by applying coding
style checks and automated rewrites throughout the authoring process
(pre-commit, commit, review, landing).
We’re announcing the following changes to our coding style and its
enforcement through our commit policy:
- We will migrate to the Prettier coding style to encourage more
consistent code. We will be preserving all other aspects pertaining
to code quality (linting) through eslint. - We will check conformance with coding style rules upon review and
landing, so that issues can be easily addressed or fixed through
automation. The preference will be to enforce style issues as early
as possible (before landing) to avoid late surprises. - We will automatically enforce restrictions on formatting of
whitespace (such as indentation and braces). For other stylistic
issues (such as naming), no particular style is enforced, and
consistency with surrounding code should take precedence.
These changes have been approved both by Firefox senior engineering
leadership and the Firefox Technical Leadership Module
https://wiki.mozilla.org/Modules/Firefox_Technical_Leadership.
We’ve already smoke-tested these changes, as well as fine tuned
various aspects of the tooling with the DevTools and Activity Stream
teams.
When?
A month ago, we pushed a patch
https://bugzilla.mozilla.org/show_bug.cgi?id=1551218 to reformat
and enable CLI and automation support for Prettier in a section of
the tree (devtools/debugger). This first step was intended to be a
smoke test in a controlled environment with a team of developers who
have volunteered to help us test this change. We haven’t found any
major surprises.
Last Friday we landed
https://bugzilla.mozilla.org/show_bug.cgi?id=1556013 top-level
support for Prettier, as a preliminary stop-gap for allowing folks to
check out what ./mach lint --fix does for them, and manually
whitelist sources in the meantime if needed.
Assuming that everything continues to go well, on Friday, July 5, we
will push a patch to mozilla-central in order to rewrite the entire
tree using Prettier.
We want to do this work right before a merge (when nightly version
moves to beta) to limit the impact on the beta uplift process. To
mitigate the impact on the developers who backport fixes to ESR, two
weeks after the merge we will also reformat the ESR68 code base. A
more detailed roadmap is available here
https://docs.google.com/document/d/1qV3aULyhulHsNHvnlbgAxeqlMGnpklUnxmpnY1OovXk/edit#.
Next week during Whistler, we also invite you to the “Using Prettier
across Mozilla” lightning talk to discuss in person about this project.
How?
To reformat code locally after the landing on July 5:
$ ./mach lint <glob> --fix
Eslint integrations for most popular code editors already allow
developers to easily format the code they’re working on at the time
they’re making changes to it. Prettier leverages this infrastructure
and is triggered automatically when running eslint --fix. If your
code editor has an eslint plugin, enable auto-fix to trigger
Prettier. We’ll share some tutorials about this soon.
Providing Feedback
This post is intended as an announcement, but we do welcome your
feedback on this, both at a high level and at the technical level.
For high level feedback please reach out to vporof@mozilla.com
mailto:vporof@mozilla.com. For technical feedback (e.g. bugs about
the conversion process) please file bugs under bug prettier-format
https://bugzilla.mozilla.org/show_bug.cgi?id=1558517.
We understand that coding styles can be subjective. Many people would
agree that having a consistent style across the code base is
valuable, and that’s not where we are now. This change doesn’t mean
that the coding style will remain forever this way; it gives us the
opportunity to easily change our coding style and apply the change on
the code base. It’s the first step to actually having consistency.
Thanks to the many who have helped out technically and with planning,
including Dave Townsend, Mark Banner, Dan Mosedale, Kate Hudson, Ed
Lee, Jason Laster, Sylvestre Ledru, Andi-Bogdan Postelnicu, Sebastian
Hengst and Ritu Kothari.
Victor
FAQ:
- Can I see what it looks like?
To reformat code locally today, apply the patches in bug 1558517
https://bugzilla.mozilla.org/show_bug.cgi?id=1558517 and whitelist
the respective sources in the top level .prettierignor
https://dxr.mozilla.org/mozilla-central/source/.prettierignoree
https://dxr.mozilla.org/mozilla-central/source/.prettierignore
file, then:
$ ./mach lint <glob> --fix
A formatted repository is available on Github:
https://github.com/victorporof/gecko-dev/tree/prettier to visualise
the changes.
- Can I ignore just a file?
If you have a good reason, yes. Just add it into .prettierignore
https://dxr.mozilla.org/mozilla-central/source/.prettierignore with
a comment explaining why it should be ignored.
- Can I ignore a whole directory?
If it’s third party code, yes! Just add it into thirdpartypaths.txt
https://dxr.mozilla.org/mozilla-central/source/tools/rewriting/ThirdPartyPaths.txt?q=thirdpartypaths.txt&redirect_type=direct
and mach will ignore it. Otherwise, you can also add a glob to
.prettierignore
https://dxr.mozilla.org/mozilla-central/source/.prettierignore.
- Why not continue using eslint?
To a great degree, we’ve moved closer towards a consistent codebase
using eslint. However, eslint specializes in code quality checks, and
not formatting.
For example, auto-fix is not available for most stylistic issues
https://eslint.org/docs/rules/#stylistic-issues. Another problem
pertains to output being inconsistent depending on the initial
styling. Furthermore, depending on which rules are chosen, conflicts
may also arise and successive runs can result in differently styled
output. All of those issues are incompatible with our goals. To
enforce a coding style, consistency and predictability are necessary,
both of which are not possible with eslint.
We’ll continue having eslint coexisting with Prettier. Rules strictly
pertaining to code quality will be preserved, and this task will
continue to be in eslint's domain. Most styling-related rules will be
removed in favour of separating this concern into Prettier’s domain.
Both tools will be running in tandem.
- Why not something else?
Our choice of Prettier was informed by meticulously analyzing
existing JS formatters, including eslint, jsbeautify and
clang-format. We’ve tested reformatting mozilla-central using each
one of them, measuring consistency (“is the output predictable
regardless of source?”), integration effort (“can this fit into our
CLI and build infrastructure?”), build/server time cost (“is this
fast enough?”), capabilities (“can this parse our funny-looking JS or
JSX?”) and also configurability (“can this tool suit our existing
code quality checks?”).
In addition, every single rule for all of those tools was also
individually tested against all of mozilla-central, measuring code
churn. We’ve gathered a lot of info, and decided that a minimal
configuration
https://dxr.mozilla.org/mozilla-central/source/.prettierrc is the
least impactful objectively: all other configuration rules have
relatively equal churn across the codebase regardless of the settings
used, so we defined those as subjective and settled for the defaults.
- Will this break blame on VCS?
As with the clang-format rollout for reformatting C++, we will tag
the formatting changeset commit message with “skip-blame” so that
Mercurial would automatically ignore the reformat changeset for blame
operations.
- Will I have to rebase?
In order to mitigate the impact for pending changes, we will
re-enable format-source
https://pypi.python.org/pypi/hg-formatsource and make it work with
Prettier, to locally reformat the changes before the big patch is
pulled. This will fix most of the conflict issues.
We will share more details about this later.
firefox-dev mailing list
firefox-dev@mozilla.org mailto:firefox-dev@mozilla.org
https://mail.mozilla.org/listinfo/firefox-dev
An update on auto-formatting JS code in comm-central.
As discussed in the bug
https://bugzilla.mozilla.org/show_bug.cgi?id=1572047
the plan is to do the big auto-formatting pass on Thursday Aug 29 so
that it is in place before the next merge on Monday Sept 2nd beta. That
will make any uplifts to the next beta release simpler.
There is a proof of concept patch on the bug where you can see what the
changes will look like. (part2-wip-prettier-addrbook-1.patch) By design
Prettier (the auto-formatting library) has only limited customization
options, and most are already determined by our eslint rules. However,
if there are discussions to be had on this front let's go ahead and have
them.
Of course, not everyone will agree with every aspect of Prettier's code
style (or any code style for that matter), but the benefits of automated
code formatting (productivity, consistency, etc.) are worth making
compromises on preferred code style.
Third party libraries like those used in chat will be ignored and not
auto-formatted. (I assume that ical.js and jsmime.js will become
auto-formatted, but I need to confirm that.)
After the auto-formatting pass on trunk we will then do an
auto-formatting pass on the TB 60 ESR branch to make backports work
smoothly.
Any thoughts or questions are welcome.
-Paul
On 8/7/19 7:43 AM, Magnus Melin wrote:
This is definitely something we need to follow through with in
comm-central too.
I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1572047 to
implement this change and running a formatting pass across the code base.
It should be a pretty good productivity boost as reviews no longer
need to care much about code formatting issues.
-Magnus
I'm not sure if any development happens there, but technically jsmime
and ical.js are separate projects [1][2], we should either apply the
auto-formatting there as well, or not auto-format it (in my opinion). We
also could say we don't care about that being a separate project
anymore, in which case we could archive it or something.
I'm sure Philipp and Joshua would have opinions about this.
--Patrick
[1] https://github.com/mozilla-comm/jsmime
[2] https://github.com/mozilla-comm/ical.js
On 8/20/19 9:59 AM, Paul Morris wrote:
An update on auto-formatting JS code in comm-central.
As discussed in the bug
https://bugzilla.mozilla.org/show_bug.cgi?id=1572047
the plan is to do the big auto-formatting pass on Thursday Aug 29 so
that it is in place before the next merge on Monday Sept 2nd beta.
That will make any uplifts to the next beta release simpler.
There is a proof of concept patch on the bug where you can see what
the changes will look like. (part2-wip-prettier-addrbook-1.patch) By
design Prettier (the auto-formatting library) has only limited
customization options, and most are already determined by our eslint
rules. However, if there are discussions to be had on this front
let's go ahead and have them.
Of course, not everyone will agree with every aspect of Prettier's
code style (or any code style for that matter), but the benefits of
automated code formatting (productivity, consistency, etc.) are worth
making compromises on preferred code style.
Third party libraries like those used in chat will be ignored and not
auto-formatted. (I assume that ical.js and jsmime.js will become
auto-formatted, but I need to confirm that.)
After the auto-formatting pass on trunk we will then do an
auto-formatting pass on the TB 60 ESR branch to make backports work
smoothly.
Any thoughts or questions are welcome.
-Paul
On 8/7/19 7:43 AM, Magnus Melin wrote:
This is definitely something we need to follow through with in
comm-central too.
I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1572047 to
implement this change and running a formatting pass across the code
base.
It should be a pretty good productivity boost as reviews no longer
need to care much about code formatting issues.
-Magnus
Hello Paul,
thanks for the examples. It helps to see it concretely.
I'm surprised by the bad results. In which world is:
var { MailServices } = ChromeUtils.import(
<a class="moz-txt-link-rfc2396E" href="resource:///modules/MailServices.jsm">"resource:///modules/MailServices.jsm"</a>
);
better than
var {MailServices} = ChromeUtils.import(<a class="moz-txt-link-rfc2396E" href="resource:///modules/MailServices.jsm">"resource:///modules/MailServices.jsm"</a>);
? The rest is similarly destructive.
I am all for adding {} around 1-line if statements, and if we can do that automatically, that's nice. But
I notice 2 general problem categories:
- The Prettier is badly configured. In the example above, it should not put the parameter on its own line. Similarly, if there are literal array initializers with a single element, they should stay as-is.
- The Prettier is overzealous, leaving no room to express semantics. This is particularly important in
ifexpressions. Often, you have 2 conditions to check, but each condition has a null-check before the actual check. Writing this on 2 lines instead of 4 lines, with each of these conditions together with the null check, increases code readability a lot. Forcing them to be on separate lines breaks the semantic connection and is destructive. So, such decisions should be left to the original developer (not the reviewer, and not the tool), and shouldn't be subject to discussion.
Ben
Paul Morris wrote on 20.08.19 15:59:
An update on auto-formatting JS code in comm-central.
As discussed in the bug
https://bugzilla.mozilla.org/show_bug.cgi?id=1572047the plan is to do the big auto-formatting pass on Thursday Aug 29 so that it is in place before the next merge on Monday Sept 2nd beta. That will make any uplifts to the next beta release simpler.
There is a proof of concept patch on the bug where you can see what the changes will look like. (part2-wip-prettier-addrbook-1.patch) By design Prettier (the auto-formatting library) has only limited customization options, and most are already determined by our eslint rules. However, if there are discussions to be had on this front let's go ahead and have them.
Of course, not everyone will agree with every aspect of Prettier's code style (or any code style for that matter), but the benefits of automated code formatting (productivity, consistency, etc.) are worth making compromises on preferred code style.
Third party libraries like those used in chat will be ignored and not auto-formatted. (I assume that ical.js and jsmime.js will become auto-formatted, but I need to confirm that.)
After the auto-formatting pass on trunk we will then do an auto-formatting pass on the TB 60 ESR branch to make backports work smoothly.
Any thoughts or questions are welcome.
-Paul
On 8/7/19 7:43 AM, Magnus Melin wrote:
This is definitely something we need to follow through with in comm-central too.
I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1572047 to implement this change and running a formatting pass across the code base.
It should be a pretty good productivity boost as reviews no longer need to care much about code formatting issues.
-Magnus
_______________________________________________
Maildev mailing list
Maildev@lists.thunderbird.net
http://lists.thunderbird.net/mailman/listinfo/maildev_lists.thunderbird.net
Thanks Patrick. On second thought I think it makes sense to hold off
and not auto-format these projects for the moment. There's plenty to do
for the main code base, and we can do these projects as a follow-up if
that's the decision.
I'm curious to hear Philipp and Joshua's thoughts, but I'll plan on not
auto-formatting these projects for now.
-Paul
On 8/20/19 10:03 AM, Patrick Cloke wrote:
I'm not sure if any development happens there, but technically jsmime
and ical.js are separate projects [1][2], we should either apply the
auto-formatting there as well, or not auto-format it (in my opinion). We
also could say we don't care about that being a separate project
anymore, in which case we could archive it or something.
I'm sure Philipp and Joshua would have opinions about this.
--Patrick
Hello Ben,
Thanks for your thoughts. We can add {} around 1-line if statements
automatically, so that is indeed nice. But as I mentioned, Prettier (by
design, for better or worse) has very limited config options[0].
I believe all the things you mentioned are the result of how Prettier
re-formats lines that are longer than the 80 character default. (The
exact line width is configurable.) It has consistent ways it breaks up
lines that are too long.
For example, if statements are always broken up with one conditional
expression per line. The idea is that having that consistency improves
overall readability because you always know what to expect.
There is an escape hatch that developers can use to override the
auto-formatting: "A JavaScript comment of|// prettier-ignore| will
exclude the next node in the abstract syntax tree from formatting."[1]
That would work for your "if" example with the null-checks.
This morning I attempted unsuccessfully to use eslint's config options
to get Prettier to ignore the 80 character line width for these import
lines. Unfortunately I don't think that is possible.
Since we don't have a lot of options, here is where I make an appeal for
flexibility on code style preferences so we can get the benefits of
auto-formatting.
-Paul
[0] https://prettier.io/docs/en/options.html
[1] https://prettier.io/docs/en/ignore.html
On 8/20/19 10:16 AM, Ben Bucksch wrote:
Hello Paul,
thanks for the examples. It helps to see it concretely.
I'm surprised by the bad results. In which world is:
var { MailServices } = ChromeUtils.import(
"resource:///modules/MailServices.jsm"
);
better than
var {MailServices} = ChromeUtils.import("resource:///modules/MailServices.jsm");
? The rest is similarly destructive.
I am all for adding {} around 1-line if statements, and if we can do
that automatically, that's nice. But
I notice 2 general problem categories:
- The Prettier is badly configured. In the example above, it should
not put the parameter on its own line. Similarly, if there are
literal array initializers with a single element, they should stay
as-is. - The Prettier is overzealous, leaving no room to express semantics.
This is particularly important inifexpressions. Often, you
have 2 conditions to check, but each condition has a null-check
before the actual check. Writing this on 2 lines instead of 4
lines, with each of these conditions together with the null check,
increases code readability a lot. Forcing them to be on separate
lines breaks the semantic connection and is destructive. So, such
decisions should be left to the original developer (not the
reviewer, and not the tool), and shouldn't be subject to discussion.
Ben
Paul Morris wrote on 20.08.19 15:59:
An update on auto-formatting JS code in comm-central.
As discussed in the bug
https://bugzilla.mozilla.org/show_bug.cgi?id=1572047
the plan is to do the big auto-formatting pass on Thursday Aug 29 so
that it is in place before the next merge on Monday Sept 2nd beta.
That will make any uplifts to the next beta release simpler.
There is a proof of concept patch on the bug where you can see what
the changes will look like. (part2-wip-prettier-addrbook-1.patch) By
design Prettier (the auto-formatting library) has only limited
customization options, and most are already determined by our eslint
rules. However, if there are discussions to be had on this front
let's go ahead and have them.
Of course, not everyone will agree with every aspect of Prettier's
code style (or any code style for that matter), but the benefits of
automated code formatting (productivity, consistency, etc.) are worth
making compromises on preferred code style.
Third party libraries like those used in chat will be ignored and not
auto-formatted. (I assume that ical.js and jsmime.js will become
auto-formatted, but I need to confirm that.)
After the auto-formatting pass on trunk we will then do an
auto-formatting pass on the TB 60 ESR branch to make backports work
smoothly.
Any thoughts or questions are welcome.
-Paul
On 8/7/19 7:43 AM, Magnus Melin wrote:
This is definitely something we need to follow through with in
comm-central too.
I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1572047 to
implement this change and running a formatting pass across the code
base.
It should be a pretty good productivity boost as reviews no longer
need to care much about code formatting issues.
-Magnus
--
Paul Morris
Thunderbird.net
For anyone curious, I found a few cases where our current eslint
settings conflict with Prettier. More details and proposed resolutions
in this comment on the bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1572047#c13
-Paul