-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft OEP-33: Code Autoformatting #107
Changes from 7 commits
ce5df36
36ecb2e
7233c05
2aeee40
2815c26
d5bccd6
56981d0
4514c89
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
=========================== | ||
OEP-33: Code Autoformatting | ||
=========================== | ||
|
||
+-----------------+--------------------------------------------------------+ | ||
| OEP | :doc:`OEP-33 </oeps/oep-0033-bp-code-autoformatting>` | | ||
+-----------------+--------------------------------------------------------+ | ||
| Title | Code Autoformatting | | ||
+-----------------+--------------------------------------------------------+ | ||
| Last Modified | 2019-03-04 | | ||
+-----------------+--------------------------------------------------------+ | ||
| Authors | Calen Pennington <cale@edx.org> | | ||
+-----------------+--------------------------------------------------------+ | ||
| Arbiter | Ned Batchelder <ned@edx.org> (Tentatively) | | ||
+-----------------+--------------------------------------------------------+ | ||
| Status | Draft | | ||
+-----------------+--------------------------------------------------------+ | ||
| Type | Best Practice | | ||
+-----------------+--------------------------------------------------------+ | ||
| Created | 2019-03-04 | | ||
+-----------------+--------------------------------------------------------+ | ||
| `Review Period` | 2019-03-04 - 2019-04-01 | | ||
+-----------------+--------------------------------------------------------+ | ||
|
||
Context | ||
------- | ||
|
||
In a large software project or ecosystem like the OpenEdX codebase, consistency | ||
of code formatting makes it easier to read the code. In an ecosystem with many | ||
contributors, maintaining that consistency requires automated tools (such as | ||
linters), consistent code review, or both. In general, minimizing the number | ||
of review comments that a reviewer has to make on an incoming change makes | ||
review easier, and lessens the number of back-and-forths required to merge | ||
the code. As such, linters that can be run locally should be preferred. | ||
Similarly, ambiguity in style guidelines should be minimized, so that there | ||
are fewer places for commentary around what amounts to personal preference | ||
(which might then instigate unnecessary debate). | ||
|
||
In short, more consistent, less ambiguous standards for code formatting | ||
make for more productive code reviews and waste less time on inconsequential | ||
details. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have there been recent incidents where inconsistent formatting has resulted in longer PR reviews? I have not found this to be a major issue in my own personal experience to-date working on our platform. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I run into pylint violations that are simply a question of spacing/indentation fairly frequently. I also often refrain from commenting on formatting that would make the code more consistent because it's not something that I want to have another review cycle on. Maybe it's just me that's bothered by inconsistent code formatting? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, right. I've also run into pylint violations, complaining of extra spaces that it doesn't allow. We could also just update our linter to ignore that violation if it's not necessary to enforce. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My view is that there is value in consistency and in the spacing guidelines that pylint suggests, from a readability perspective, and that there isn't much value in making engineers actually enforce it themselves, since tooling can do that job faster and more effectively. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @nasthagiri, great question. Yes, formatting and CI in general has been a painpoint for me while contributing to Open edX. Despite its great value, it still puts a lot of unrelated issues to the code that I write -- say I edited line no. 1, it would complain about line no. 100. This is especially true for home-grown linters such as the XSS linter. I'm not saying that we should loosen and "de-regulate" Open edX quality, but at the same time please don't underestimate the effort that infrequent contributors have to go through. This is especially true for the I have learned my way through the process, but I find it hard to recruit other open source contributors whether from my company or from the community. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a separate OEP (#108) that I want to use to cut off the accidental flagging from the XSS Linter. In general, I'm all for making it so that the only things that actually get flagged on a PR are substantive issues in code that you actually wrote in the PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @cpennington! That would be great. Afterwards, we'd only be left with the inevitable cost of dealing with the CI on such a monolithic piece of code -- the Therefore, I think we should look at such initiatives a bit more critically that we're doing now -- which is mostly focusing on the pro's of adding yet another step. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the impact on time-to-feedback for edx-platform specifically is a good question. On my machine (not devstack), w/ no black cache, it takes 10 minutes to format all of edx-platform. After reformatting everything, it takes 13s to reformat/check again. After I reformat everything, and then delete the cache, a full format takes 5 minutes (and again, once cached, takes about 13s). So, that suggests that a) we want to make sure jenkins keeps that cache around, and b) that devstacks are pre-built with the cache so that running black inside devstack to do a full-format (for someone w/o editor integration) is fast enough to not be a blocker. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sigh... turns out that black needs python 3.6 to run, which we don't have installed on devstack... So, we'd need some more investment to make edx-platform work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Internally at Stanford, I enforce this fairly rigidly, through sheer force of will :) |
||
|
||
Decision | ||
-------- | ||
|
||
We will select automatic code formatters for all of our major languages. These | ||
formatters will have both a lint-mode (that will be run in continuous | ||
integration to ensure consistent formatting) and an autoformat mode (which | ||
can be integrated with editors to free developers from having to think about | ||
code formatting). These formatting standards will be applied across all | ||
OpenEdX-authored repositories. | ||
|
||
Selected tools should, ideally, require minimal configuration. Any required | ||
configuration should be added to `edx/edx-lint`_ in order to allow it to | ||
be centralized and standardized across repositories in the OpenEdX ecosystem. | ||
|
||
The current recommended formatters are: | ||
|
||
- Python: `black`_ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Last I looked into it, Black was too opinionated and barely, if at all configurable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, opinionated is actually the goal, here. Whatever formatter is chosen should format any given code construct identically no matter how it's originally input. I think the pain of the rewrite is going to be there no matter how big or small it is (there's gonna be one big PR against the repo either way). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
My hangup here isn't being opinionated; I can rerun my test cases again if you'd like the specific violations, There where specific style patterns that we chose intentionally to make our fork more manageable, but we were no longer able to use these patterns under
I'm not suggesting flexibility for the sake of endless and ongoing style debates In RFC terms, think
Agreed.
It's really a matter of perspective...
Now when it comes to running it against existing platform code, consider:
versus
I concede, single shot is easiest for you, it's one run of an automated tool. And a staged roll out let's us absorb those hits over time, ideally over the course of named releases. When there have been sweeping changes to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, that's fair enough. Like I said, I hadn't run into places where I objected w/ Black's formatting, so it would be helpful to me (while making a recommendation in this OEP) to see your specific counterexamples.
I'm definitely willing to defer to your experience on this. It shouldn't be too much trouble to set up our integration tooling in a way that we can mark specific directories as having been |
||
- Javascript: `prettier`_ | ||
- CSS/SASS: `prettier`_ | ||
- HTML: `prettier`_ | ||
- JSON: `prettier`_ | ||
- PHP: `prettier-php`_ | ||
|
||
.. _black: https://github.com/ambv/black | ||
.. _prettier: https://prettier.io/ | ||
.. _prettier-php: https://github.com/prettier/plugin-php | ||
.. _`edx/edx-lint`: https://github.com/edx/edx-lint | ||
|
||
Formatting should exposed via the standard ``Makefile`` commands | ||
``format`` (which should autoformat all code in the repository) | ||
and ``check-format`` (which should validate that all code in the | ||
repository is formatted). ``check-format`` should also be called | ||
as part of ``make quality``. | ||
|
||
When bulk-formatting the entire repo for the first time, all changes should | ||
be done in as few commits as possible (ideally, only one). Then, the | ||
commit hashes should be added to a ``.git-blame-ignore-revs`` file. | ||
This will allow the `git hyper-blame`_ tool to ignore them by default, | ||
to preserve the easy ability to see who changed particular file lines. | ||
|
||
.. _git hyper-blame: https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While this seems interesting, I don't think it helps for many use cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, Github doesn't support it. I'm willing to include it if people want it (or even if it would be marginally useful), because it isn't a big lift to add (but I agree, it's also doesn't solve a lot of use-cases). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I guess I'm not saying don't mention it, as much as I wanted to be clear that it shouldn't be interpreted as end-all solution. |
||
|
||
Consequences | ||
------------ | ||
|
||
There will be commits to all OpenEdX repositories that cause large-scale | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should know how to spell Open edX by now :) |
||
reformatting without functional changes. This will muddy the blame-file | ||
history. Users with auto-formatters configured in their editors working | ||
in a repo that is not yet OEP-33 compliant will need to disable their | ||
autoformatter, or risk large-scale changes that are unrelated to their | ||
current tasks. All OpenEdX code will be formatted consistently and | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do editors generally have a way to configure auto-formatting on a per-repo basis? Is that something we can configure centrally? This will help with gradual adoption of auto-formatting across Open edX. Perhaps more importantly, many developers work on non-Open-edX projects also, so universal auto-formatting is not reasonable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure about all editors, but VSCode has per-workspace (aka per-repo) settings that can disable autoformatting (or, conversely, could be used to enable autoformatting only on particular repos). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Many of my concerns can be alleviated if things were to be rolled out on a per-repo basis. |
||
unambiguously. Developers will not have arguments about | ||
code layout. | ||
|
||
References | ||
---------- | ||
|
||
`gofmt`_ was the first code-autoformatter to gain wide-scale acceptance. | ||
|
||
.. _gofmt: https://blog.golang.org/go-fmt-your-code | ||
|
||
`Black editor integration`_ | ||
`Prettier editor integration`_ | ||
|
||
.. _Black editor integration: https://github.com/ambv/black#editor-integration | ||
.. _Prettier editor integration: https://prettier.io/docs/en/editors.html | ||
|
||
An example ``tox.ini``: | ||
|
||
.. code-block:: | ||
|
||
[testenv:format] | ||
basepython = | ||
python3.6 | ||
deps=black | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From an OEP-18 perspective, this should use a requirements file generated by |
||
commands= | ||
black . | ||
|
||
[testenv:check-format] | ||
basepython = | ||
python3.6 | ||
deps=black | ||
commands= | ||
black . --check | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1