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

Introduce black linting #6586

Closed
wants to merge 6 commits into from
Closed

Conversation

webbyfox
Copy link
Contributor

@webbyfox webbyfox commented Apr 13, 2019

  • Added black configuration -> f2eacd3
  • Run black to fix formatting -> f418b4e

@webbyfox webbyfox changed the title Introduce black lintting Introduce black linting Apr 13, 2019
@webbyfox
Copy link
Contributor Author

@tomchristie, Seems like they always released alpha/beta versions till the date as they admit it not stable yet but the number of projects using it.
https://black.readthedocs.io/en/stable/installation_and_usage.html#note-this-is-a-beta-product

@tomchristie
Copy link
Member

So, I really like this, but I am finding it hard to correctly review without having the "introduce to the linting" split out from the "apply black".

It might be a good idea for us to start with a failing test case that only adds black into the linting, but doesn't actually apply it.

That way I can review it more thoroughly.

@rpkilby
Copy link
Member

rpkilby commented Apr 17, 2019

Just throwing in my 2 cents, but I am -1 on black. In general, I'm really biased against black, but I also think it would be somewhat disruptive to introduce it this late into the project's history. Thoughts:

  • Black is automatic, but it's not magic. A lot of the formatting/style decisions it makes don't really improve readability. This is less of an issue when black is used form the start, where you can work with black to produce conformant code that is also readable. But from what I've seen, applying black en masse on mature projects reduces readability in a lot of cases. It's a bit of a mixed bag.
  • Git blame is always useful for doing code archaeology. Black is going to muck with this.
  • It will be tedious to audit changes on 136 files and ensure that functionality hasn't been changed.

If we do want to go with black though, per the third point, we'd probably want someone from within the org (e.g., @tomchristie) to apply black so we can trust that there aren't any erroneous/additional changes, unintentional or otherwise (e.g., bugs in black).

We may also want to consider using --skip-string-normalization, which is intended for large, pre-existing projects like DRF. This would help cut down on a lot of the quote-changing noise that mucks with the git blame.

@tomchristie
Copy link
Member

I think the points re. git history, and impact on large mature codebases are valid.
I'm not sure if we do or don't want to adopt black, but it's certainly true that we should just have a pull request adding it in without applying the changes, and that if we do run with it, then I'll be the one applying it initially.

@webbyfox
Copy link
Contributor Author

OK, @tomchristie, I will create separate PR for applying black configuration soon.


ret = renderer.render(self.data, accepted_media_type, context)
if isinstance(ret, six.text_type):
assert charset, (
'renderer returned unicode, and did not specify '
'a charset value.'
"renderer returned unicode, and did not specify " "a charset value."
Copy link
Contributor

Choose a reason for hiding this comment

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

These two strings can be one now. This is a known upstream bug psf/black#26.

self.assertEqual(
1,
len(s.errors["address"]),
"Unexpected number of validation errors: " "{0}".format(s.errors),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, can merge.

@rpkilby
Copy link
Member

rpkilby commented May 1, 2019

Closing per #6586 (comment) and the creation of #6630.

@rpkilby rpkilby closed this May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants