Skip to content
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

Add Black compatible configurations in documentation (#1366 & #1205) #1371

Merged
merged 11 commits into from
May 8, 2020

Conversation

ichard26
Copy link
Collaborator

@ichard26 ichard26 commented Apr 29, 2020

Resolves #1366 and resolves #1205.

Please be warned, this is my first pull request and my first attempt at writing decent documentation.
So feel free to mention if I've done anything wrong or if my documentation is lacking in quality.

Note that this PR isn't ready for review since some work needs to be finished:

  • I need to add references in README. I haven't done it yet since I'm too scared of touching the README ... It's the front page of this project.
  • I should add explanations for the configurations since copying and pasting code you don't understand is always a horrible idea.

Also, while I did test all of the configurations to verify that they work with their respective tools, I may have missed a few options. I went for the least amount of configuration possible.

Once again, any comments/suggestions on my PR, my commit messages, or my actual documentation are welcome! Also so are comments/suggestions on how I should link from the README.

P.S: Thanks for this tool, it's really useful!

When users of Black are starting a new project or are adding Black to their
existing projects, they will usually have to config their existing tools like
flake8 to be compatible with Black. Usually, these configs are the same across
different projects. Yet, there isn't any consolidated infomation on what configs
are compatible. Leaving users of Black to dig out any infomation they can find
in README.md.
@ichard26 ichard26 changed the title Add Black compatible configs in docs (#1366 & #1205) Add Black compatible configurations in documentation (#1366 & #1205) Apr 29, 2020
@ichard26
Copy link
Collaborator Author

b914650 Whoops, looks like I missed changing my testing value. max-line-length = 79 is now fixed to max-line-length = 88.

Copying and pasting code without understanding it is a bad idea. This goes the same
for users copying and pasting configurations.
docs/compatible_configs.md Outdated Show resolved Hide resolved
@ichard26 ichard26 marked this pull request as ready for review May 2, 2020 19:08
ichard26 added 3 commits May 6, 2020 14:27
I referenced the wrong source. I used a pesonal configuration, not a pure Black-
compatible configuration.
The explanation for the isort configuration was pretty bad. After having fixed the
configuration (see commit 01c55d1), improving the its explanation was necessary to
make it more user-friendly and understandable. Also added fenced code blocks of the
raw configuration options so the explanations make sense.
@cooperlees cooperlees self-assigned this May 8, 2020
Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. I just slightly dislike the direct call out to isort. I might merge and change this to be a little more gentle and we get another maintainer's approval.


While _Black_ enforces formatting that conforms to PEP 8, other tools may raise warnings
about _Black_'s changes or will overwrite _Black_'s changes... I am looking at you
[isort](https://pypi.org/p/isort). Since _Black_ is barely configurable, these tools
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should call out isort directly. It was never designed to work with black. Can we remove this please or reword to be a little more subtle.
"A good example here is isort"

Copy link
Collaborator Author

@ichard26 ichard26 May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. In hindsight, isort and Black were never designed to be compatible with each other and it's unfair to call out isort for that. I'll try to reword it to more gentle or just remove it if I fail at doing that.

@@ -0,0 +1,261 @@
# _Black_ compatible configurations

Most of the changes that _Black_ make are harmless, but a few do conflict against other
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of Black's changes are harmless (or at least, they should be)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well noted, I think I've been stalking the issue section a bit too much.

[flake8](https://pypi.org/p/flake8/) is a code linter. It warns you of syntax errors,
possible bugs, stylistic errors, etc. For the most part, flake8 follows
[PEP 8](https://www.python.org/dev/peps/pep-0008/) when warning about stylistic errors.
Except, there are a few deviations that cause incompatiblities with _Black_.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Except, there are a few deviations that cause incompatiblities with _Black_.
There are a few deviations that cause incompatibilities with _Black_.

)
```

Although, while this style is PEP 8 compliant, Pylint will raise
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Although, while this style is PEP 8 compliant, Pylint will raise
Although this style is PEP 8 compliant, Pylint will raise

Copy link
Contributor

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


</details>

## flake8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## flake8
## Flake8


## flake8

[flake8](https://pypi.org/p/flake8/) is a code linter. It warns you of syntax errors,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[flake8](https://pypi.org/p/flake8/) is a code linter. It warns you of syntax errors,
[Flake8](https://pypi.org/p/flake8/) is a code linter. It warns you of syntax errors,

## flake8

[flake8](https://pypi.org/p/flake8/) is a code linter. It warns you of syntax errors,
possible bugs, stylistic errors, etc. For the most part, flake8 follows
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
possible bugs, stylistic errors, etc. For the most part, flake8 follows
possible bugs, stylistic errors, etc. For the most part, Flake8 follows

`W503 line break before binary operator` warnings.

In some cases, as determined by PEP 8, _Black_ will enforce an equal amount of
whitespace around slice operators. Due to this, flake8 will raise
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
whitespace around slice operators. Due to this, flake8 will raise
whitespace around slice operators. Due to this, Flake8 will raise

whitespace around slice operators. Due to this, flake8 will raise
`E203 whitespace before ':'` warnings.

Since both of these warnings are not PEP 8 compliant, flake8 should be configured to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Since both of these warnings are not PEP 8 compliant, flake8 should be configured to
Since both of these warnings are not PEP 8 compliant, Flake8 should be configured to

Since both of these warnings are not PEP 8 compliant, flake8 should be configured to
ignore these warnings via `extend-ignore = E203, W503`.

Also, as like with isort, flake8 should be configured to allow lines up to the length
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Also, as like with isort, flake8 should be configured to allow lines up to the length
Also, as like with isort, Flake8 should be configured to allow lines up to the length


## Pylint

[Pylint](https://pypi.org/p/pylint/) is also a code linter like flake8. It has the same
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[Pylint](https://pypi.org/p/pylint/) is also a code linter like flake8. It has the same
[Pylint](https://pypi.org/p/pylint/) is also a code linter like Flake8. It has the same

@zsol zsol added the T: documentation Improvements to the docs (e.g. new topic, correction, etc) label May 8, 2020
@cooperlees cooperlees merged commit 8c3c190 into psf:master May 8, 2020
@cooperlees
Copy link
Collaborator

@ichard26 - I made the changes and merged! Thanks. We're having a black maintainers merge-a-thon today to try get black to become a python package - as per #1376

@ichard26
Copy link
Collaborator Author

ichard26 commented May 8, 2020

Oh well, I was going to add a commit with your very well noted suggestions but looks like you beat me to it. Thank you for putting in time for the PR regardless! It was a good first experience for contributing to open source.

We're having a black maintainers merge-a-thon

Oh yeah I can tell, my poor little email is getting spammed... good sign though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: documentation Improvements to the docs (e.g. new topic, correction, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bring pylint to its senses with a .pylintrc Document "black-compatible" configs for common linters/fixers
5 participants