Migrate to black #1039
Merged
Migrate to black #1039
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
tests/model/test_model.py
Outdated
| @@ -17,6 +17,7 @@ | |||
|
|
|||
|
|
|||
| CONTROLLER = 'zulipterminal.core.Controller' | |||
| MODEL = 'zulipterminal.model.Model' | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
neiljp
force-pushed
the
2021-05-26-black-hoisted
branch
from
a8ef8fc to
a1c5373
Compare
zee-bit
approved these changes
May 29, 2021
neiljp
force-pushed
the
2021-05-26-black-hoisted
branch
2 times, most recently
from
bdfe608 to
cd017ed
Compare
preetmishra
approved these changes
May 30, 2021
There was a problem hiding this comment.
@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.
neiljp
force-pushed
the
2021-05-26-black-hoisted
branch
from
cd017ed to
5a98b35
Compare
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
force-pushed
the
2021-05-26-black-hoisted
branch
from
5a98b35 to
b8e910d
Compare
|
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! 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR comprises:
blackexperience.pyproject.tomltools/lint-all(somake checkandmake lint) and fixing tomake fix