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

Use black to automatically format our code #1355

Closed
wants to merge 1 commit into from

Conversation

daviddavis
Copy link
Contributor

@daviddavis daviddavis commented May 23, 2019

[noissue]

A couple notes. You can integrate black with version control:

https://github.com/python/black#version-control-integration

Or your IDE:

https://black.readthedocs.io/en/stable/editor_integration.html

Here is django's accepted proposal for black adoption:

https://github.com/django/deps/blob/master/accepted/0008-black.rst

@daviddavis daviddavis requested a review from a team May 23, 2019 16:02
@pep8speaks
Copy link

pep8speaks commented May 23, 2019

Hello @daviddavis! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-06-04 11:54:49 UTC

@daviddavis daviddavis force-pushed the black branch 3 times, most recently from 9722698 to e5bda15 Compare May 23, 2019 16:15
@@ -269,6 +270,7 @@ def to_createrepo_c(self):
createrepo_c.Package: package itself in a format of a createrepo_c package object

"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a conflict here between black and flake8-docstrings. Black adds this line due to the nested function ("Black won’t insert empty lines after function docstrings unless that empty line is required due to an inner function starting immediately after") while flake8-docstrings enforces a rule that there should be no blank line after docstrings (D202).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am ignoring D202 for now until PyCQA/pydocstyle#361 (comment) is addressed.

Copy link

@nixocio nixocio left a comment

Choose a reason for hiding this comment

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

That is a really good change.

.travis/script.sh Outdated Show resolved Hide resolved
@goosemania
Copy link
Member

Double quotes noise makes me sad :(
It turns out there was a huge flame about it psf/black#118 but the resolution was only enable/disable string normalization.

@daviddavis
Copy link
Contributor Author

@goosemania agreed. I'm pretty indifferent to the use of single vs double but it does create a lot of noise.

@daviddavis daviddavis force-pushed the black branch 3 times, most recently from 7d5912e to 5c63830 Compare June 4, 2019 11:46
primary_xml_path = results[0].path
filelists_xml_path = results[1].path
other_xml_path = results[2].path
metadata_pb.done += 3
metadata_pb.save()

packages = await RpmFirstStage.parse_repodata(primary_xml_path,
Copy link
Member

Choose a reason for hiding this comment

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

to me, this is more readable than what black is doing.

Copy link
Member

Choose a reason for hiding this comment

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

In my opinion Black is making more readable (both Human and Machine readable)

Black could do better if instead of inline it did one argument by line with ending comma

packages = await RpmFirstStage.parse_repodata(
    primary_xml_path, 
    filelists_xml_path, 
    other_xml_path,
)

This style makes easier to run find-replace tools and also easier to analyse code in reviews.

Copy link
Contributor Author

@daviddavis daviddavis Jun 6, 2019

Choose a reason for hiding this comment

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

I agree. It seems like it should either be one line (if it fits) or multiple lines. black does appear though to follow @rochacbruno's example if the arguments don't fit on a single line.

@@ -7,6 +7,7 @@ set -veuo pipefail

# Lint code.
flake8 --config flake8.cfg || exit 1
black --diff . || exit 1
Copy link

Choose a reason for hiding this comment

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

This should be black --check --diff . || exit 1 (without --check black just outputs a diff without setting an error code when reformatting is required)

@daviddavis
Copy link
Contributor Author

This PR is stale. Will open a new PR using plugin_template.

@daviddavis daviddavis closed this Jul 29, 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

7 participants