Migrate to black #1039

neiljp · 2021-05-27T01:36:14Z

This PR comprises:

many prep commits motivated by - though not all required for - a better post-black experience.
black configuration in pyproject.toml
removal of a conflicting flake8 rule
addition of black linting to tools/lint-all (so make check and make lint) and fixing to make fix
two commits for migrating structure only, then adjusting to a standard quote style
enforcement of black linting via CI

This makes tests easier to read and more compact. (it will also avoid issues with long lines with `black` later) NOTE: The code/test complexity suggests we may want to refactor later.

Ezio-Sarthak · 2021-05-29T09:20:33Z

tests/model/test_model.py

@@ -17,6 +17,7 @@


 CONTROLLER = 'zulipterminal.core.Controller'
+MODEL = 'zulipterminal.model.Model'


I just wanted to point that I had a PR #959 that does a general simplification of patches in all tests. Let me know if I should make it review-ready if we wish to merge it first.

EDIT: I just updated that PR after rebasing and resolving conflicts.

@Ezio-Sarthak Thanks for reminding me of your PR 👍

I welcome your intent to add more constants to improve readability; the main challenge with using these is that we don't currently have much of a "standard" way of applying these constants - or it seems that way, but isn't quite. In some pre-existing cases we have files referred to, others to classes, and I think this is where we would benefit from standardizing and clarying. You'll note that in my commit I only extracted the classes, since they were both the longest - so more likely to benefit from compacting the long lines - but also that it made clear that others were module-specific, either imports or not in the main class.

preetmishra

@neiljp Thanks for putting all the work into this! This looks great. 👍

The recent increased maximum line-length allows these to be combined, which improves readability and reduced visual noise. These were originally introduced due to breaking across lines, which is still required in some cases.

This is similar to the previous commit, but also involves minimal indent adjustment to pass linting. (the minimal indenting will be adjusted automatically later upon application of black) The parametrize names being split into lists was originally introduced due to breaking across lines, which is still required in some cases.

This is possible now due to the longer maximum line-length. (`black` cannot perform this reorganizing automatically)

Increased maximum longer line-length allows these to be removed. (black won't remove these automatically later)

Trailing commas have meaning to `black` and force multi-line formatting where not necessary.

This makes it clearer which test case has which id, compared to having separate lists of test case values and ids, particularly when such lists are long. (this will potentially be of more importance when `black` is in use, when it can expand case values over many lines)

`case` prefixes added, in preparation for addition of inline ids. Two entries separated onto different lines. Other reformatting to satisfy flake8.

This will be particularly useful with `black` in use, but also clarifies which id matches which test case.

This is a precursor reformat to adding inline test ids.

Once we apply `black`, this will be reformatted; this commit moves the comment so it stays in a reasonable position.

Upon application of `black`, subsequent lines are not obviously grouped together without these.

Upon application of `black`, these test cases become vertical; adding these commas ensures the ids are also vertical rather than combining on one line.

The structure indicates the pattern of covered values, which is not evident in a single list.

These implicitly-combined multiline strings are visually clear as currently laid out; line combination as applied by `black` makes this less clear.

These can force multi-line formatting where not necessary in black.

These will force multi-line formatting when black is applied.

With the longer maximum line-length, this allows simplification of strings spanning multiple lines which are implicitly combined. Applying `black` will soon tidy this further, but does not merge adjacent implicitly joinable strings; this commit handles this manually to avoid such strings being placed manually on the same lines, or split awkwardly.

With the longer maximum line-length some of these can be simpler. More importantly `black` will not always automatically remove these, leading to extra indenting and lines for parentheses in some cases.

These comments would be misplaced after applying `black`, so this commit manually adjust the formatting so that the comments are maintained, including one mypy type-ignore.

When reformatted with `black` these are more spread-out, but also suffer inconsistent formatting if data fits on different numbers of lines, so disable this for now using `# fmt: off/on`.

Dependency added to setup.py, configuration in new file pyproject.toml.

Achieved by running `black -S zulipterminal/ tests/`

Achieved via `black zulipterminal/ tests/`.

neiljp · 2021-06-01T03:35:34Z

Thanks for all the feedback everyone - this has been rather a lot of effort but I'm looking forward to it being useful in future! 🎉

neiljp added 3 commits May 26, 2021 17:46

refactor: tests: model: Extract CONTROLLER constant to simplify patches.

1445b54

refactor: tests: model: Extract MODEL constant to simplify patches.

7e441e0

refactor: tests: model/boxes: Extract variables to simplify tests.

2ef40d8

This makes tests easier to read and more compact. (it will also avoid issues with long lines with `black` later) NOTE: The code/test complexity suggests we may want to refactor later.

zulipbot added the size: XL [Automatic label added by zulipbot] label May 27, 2021

Ezio-Sarthak reviewed May 29, 2021

View reviewed changes

neiljp force-pushed the 2021-05-26-black-hoisted branch from a8ef8fc to a1c5373 Compare May 29, 2021 18:23

zee-bit approved these changes May 29, 2021

View reviewed changes

neiljp force-pushed the 2021-05-26-black-hoisted branch 2 times, most recently from bdfe608 to cd017ed Compare May 30, 2021 15:19

preetmishra approved these changes May 30, 2021

View reviewed changes

neiljp added 20 commits May 30, 2021 15:27

refactor: tests: run/ui/model/core: Simplify multi-line strings.

b5246ab

This is possible now due to the longer maximum line-length. (`black` cannot perform this reorganizing automatically)

refactor: tests: ui/ui_tools/model: Remove unnecessary parentheses.

a8488ef

Increased maximum longer line-length allows these to be removed. (black won't remove these automatically later)

refactor: tests: model/run/popups/boxes/ui_tools: Remove commas.

3347195

Trailing commas have meaning to `black` and force multi-line formatting where not necessary.

refactor: tests: boxes: Use standard 'case' for param import.

cefcd55

refactor: tests: ui_tools: Reformat soup2markup parametrize entries.

e638572

`case` prefixes added, in preparation for addition of inline ids. Two entries separated onto different lines. Other reformatting to satisfy flake8.

refactor: tests: ui_tools: Inline soup2markup test ids.

3a1847e

This will be particularly useful with `black` in use, but also clarifies which id matches which test case.

refactor: tests: helper: Indent outside-narrow test cases & add case.

399493f

This is a precursor reformat to adding inline test ids.

tests: helper: Add inline outside-narrow test ids.

3185035

refactor: tests: model/ui_tools: Adjust comment placement.

6117cf4

Once we apply `black`, this will be reformatted; this commit moves the comment so it stays in a reasonable position.

refactor: tests: ui_tools: Add parentheses around multi-line string.

2c4170b

Upon application of `black`, subsequent lines are not obviously grouped together without these.

refactor: tests: ui_tools: Add commas to ids for consistent formatting.

85533b8

Upon application of `black`, these test cases become vertical; adding these commas ensures the ids are also vertical rather than combining on one line.

refactor: tests: run: Disable formatting around test parametrize grid.

3d51bcd

The structure indicates the pattern of covered values, which is not evident in a single list.

refactor: tests: ui_tools: Indent & disable formatting for blockquote.

e878e20

These implicitly-combined multiline strings are visually clear as currently laid out; line combination as applied by `black` makes this less clear.

refactor: core/ui/model/tables/boxes: Remove trailing commas.

572369f

These can force multi-line formatting where not necessary in black.

refactor: run: Add trailing commas to argparse entries for consistency.

26081f3

These will force multi-line formatting when black is applied.

refactor: model/run/boxes/helper: Remove unnecessary parentheses.

937a82f

With the longer maximum line-length some of these can be simpler. More importantly `black` will not always automatically remove these, leading to extra indenting and lines for parentheses in some cases.

refactor: buttons/boxes: Reformat to maintain comment locations.

6ee42d0

These comments would be misplaced after applying `black`, so this commit manually adjust the formatting so that the comments are maintained, including one mypy type-ignore.

neiljp force-pushed the 2021-05-26-black-hoisted branch from cd017ed to 5a98b35 Compare May 30, 2021 23:16

neiljp added 9 commits May 31, 2021 14:22

refactor: keys/themes: Disable reformatting for config datastructures.

bc0aa3a

When reformatted with `black` these are more spread-out, but also suffer inconsistent formatting if data fits on different numbers of lines, so disable this for now using `# fmt: off/on`.

unicode_emojis: Update list of unicode emojis from server using script.

579bf27

unicode_emojis/tools: Update & run tool to make black-compatible data.

10a9605

linting/requirements: Add linting dependency on black & configuration.

7cd6d00

Dependency added to setup.py, configuration in new file pyproject.toml.

linting: Stop enforcing E203 via flake8, which conflicts with black.

89a1925

refactor: Reformat zulipterminal/ and tests/ with black, except quotes.

7321281

Achieved by running `black -S zulipterminal/ tests/`

refactor: Reformat zulipterminal/ and tests/ with black (incl quotes).

17c9959

Achieved via `black zulipterminal/ tests/`.

makefile/lint-all: Add support for linting & fixing formatting by black.

49e1d40

lint-and-test: Check conformance with black in CI.

b8e910d

neiljp force-pushed the 2021-05-26-black-hoisted branch from 5a98b35 to b8e910d Compare May 31, 2021 21:49

neiljp changed the title ~~WIP: Migrate to black~~ Migrate to black May 31, 2021

neiljp merged commit f79359f into zulip:main Jun 1, 2021

neiljp added this to the Next Release milestone Jun 1, 2021

neiljp added the area: infrastructure Project infrastructure label Feb 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to black #1039

Migrate to black #1039

neiljp commented May 27, 2021

Ezio-Sarthak May 29, 2021 •

edited

neiljp May 29, 2021

preetmishra left a comment

neiljp commented Jun 1, 2021

		@@ -17,6 +17,7 @@


		CONTROLLER = 'zulipterminal.core.Controller'
		MODEL = 'zulipterminal.model.Model'

Migrate to black #1039

Migrate to black #1039

Conversation

neiljp commented May 27, 2021

Ezio-Sarthak May 29, 2021 • edited

Choose a reason for hiding this comment

neiljp May 29, 2021

Choose a reason for hiding this comment

preetmishra left a comment

Choose a reason for hiding this comment

neiljp commented Jun 1, 2021

Ezio-Sarthak May 29, 2021 •

edited