Skip to content
This repository has been archived by the owner on Dec 9, 2021. It is now read-only.

Add auto-formatting and linting #135

Closed
stevejalim opened this issue Sep 16, 2019 · 6 comments · Fixed by #142
Closed

Add auto-formatting and linting #135

stevejalim opened this issue Sep 16, 2019 · 6 comments · Fixed by #142
Assignees
@stevejalim
Labels
housekeeping Non-functional, but necessary, changes

Comments

@stevejalim
Copy link
Contributor

stevejalim commented Sep 16, 2019 •
edited
Loading

Suggestion for autoformatters:

  • Python: black
  • JS: prettier (already in use)
  • CSS: prettier (already in use)
  • HTML: TBC (or nothing)

Plus linting and cleanup with:

  • isort (configuration similar to this to separate out Wagtail imports)
  • flake8
  • eslint (already in codebase)

All of this driven by pre-commit therapist
@stevejalim stevejalim added the housekeeping Non-functional, but necessary, changes label Sep 16, 2019
@escattone
Copy link
Contributor

@stevejalim Love it! We're planning to use black on kuma (but just haven't done it yet) and already use prettier. We're using flake8-import-order for enforcing import order on kuma, but after looking at isort, I like it much better!

@stevejalim
Copy link
Contributor Author

stevejalim commented Sep 16, 2019 via email •
edited
Loading

@escattone
Copy link
Contributor

@stevejalim Also, I've never used pre-commit, but we were thinking about using therapist on kuma. Do you have any thoughts on pre-commit vs. therapist?

@stevejalim
Copy link
Contributor Author

stevejalim commented Sep 16, 2019 via email •
edited
Loading

@stevejalim
Copy link
Contributor Author

stevejalim commented Sep 16, 2019 •
edited
Loading

@escattone Took a look at therapist (aided by what looks like a draft ADR gist from @peterbe 😄 and the PR about it in main mdn.)

Deliberately leaving aside the existing leaning towards therapist, I think the lack of a need for a virtualenv and the cleaner config syntax puts it ahead, so happy to go that way. Will give it a shot.

@stevejalim
Copy link
Contributor Author

stevejalim commented Sep 16, 2019 •
edited
Loading

By the way, re: isort, I remembered a decent Wagtail-friendly example of custom module, from the lovely home of Wagtail:

https://github.com/torchbox/wagtail-torchbox/blob/master/.isort.cfg

There is a risk that custom isort configs can get missed if a code editor's isort plugin doesn't by default pick up the .isort.cfg from the project dir, so you get an import tug-of-war between the editor and your CLI linter.
We'll see - it might just be solved by documenting that one's editor might need more explicit configuration to find the right config file.

@stevejalim stevejalim self-assigned this Sep 17, 2019
This was referenced Sep 17, 2019
Open
Merged
stevejalim pushed a commit that referenced this issue Sep 18, 2019
135: Support autoformatting and linting with Therapist (#142)
bcf5684 
This changeset contains a number of squashed commits towards standardising the formatting and linting of the devportal project. (It wasn't bad before, just good to get this in during a quietish patch of development).

**Please view the README and start using `therapist` to avoid other developers' commits containing formatting changes to parts of files they didn't explicitly work on**

Closes #135 

## Summarised changes: 
* full-codebase passes made with `black`, `isort`, `flake8`, `eslint` and `prettier` to bring everything into line
* add [`therapist`](https://github.com/rehandalal/therapist) as a framework for running the above tooling (either via a pre-commit hook or directly - both paths are available)

+7,255 -3,637 looks huge, but the vast majority of the diff is standardising formatting


## Squashed commits:

* 135: Full pass with `black`as first step towards autoformatting

252 files reformatted, 42 files left unchanged.

Done with `black --exclude node_modules .` to avoid unnecessarily reformatting third-party package helper Python in JS deps

Includes migration files for consistency, even though it adds noise (209/252 files are migrations...)

Tests still passing.

* 135: add isort with black-compatible config

Created a separate .isort.cfg to make it explicit

Tests passing fine after changes

* 135: tighten up flake8 to also enforce 88-char lines, matching black

This commit contains some (manual) changes to over-long lines caught by flake8 that black _didn't_ auto-fix, because they were long strings (see psf/black#182)

* 135: Run prettier across all SCSS, ahead of hooking it into therapist's pre-commit tasks

* 135: Tune prettier and eslint's ignore options

Specifically:
- prettier: ignore HTML, SVG, JSON and Python
- eslint: ignore HTML, SVG and Python

While this wouldn't be needed for editor-based formatting or linting, nor linting explicitly staged/passed files, this change is so that when we _do_ run them across the entire `developerportal` module (eg, in CI), they won't gripe about things they are not intented to be looking at anyway.

* 135: Add Therapist to the project, running the linters and formatters recently added/tweaked

At this point, the assumption is you have the following installed on your host:
* black
* flake8
* eslint and eslint-config-prettier
* isort
* prettier
(I'll deal with making that easier in a different commit.)

Install `therapist` (https://github.com/rehandalal/therapist):

	$ pip install therapist

Now install the pre-commit hook that will trigger Therapist automatically:

	$ therapist install
	Installing pre-commit hook...	DONE

Now, when you commit a change, the staged changes will be checked by one or more of black, isort, eslint and prettier, as appropriate. See `.therapist.yml` for the configuration.

Alternatively, if you wanted to run it across the whole codebase (as we might well do in CI), run:

	$ therapist run developerportal/
	black ............................................................... [SUCCESS]
	ESLint .............................................................. [SKIPPED]
	flake8 .............................................................. [SUCCESS]
	isort ............................................................... [SUCCESS]
	Prettier ............................................................ [SUCCESS]

-------------------------------------------------------------------------------
Completed in: 3.67s

And if you want therapist to auto-fix things using black, prettier and/or eslint, run:

	$ therapist run developerportal --fix

* 135: Update Readme to mention how to set up and use `therapist` as a linter/formatter runner
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@stevejalim stevejalim

Labels
housekeeping Non-functional, but necessary, changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

135: Support autoformatting and linting with Therapist mdn/developer-portal
2 participants
@stevejalim @escattone