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

Format code with black #316

Merged
merged 8 commits into from
May 7, 2021
Merged

Format code with black #316

merged 8 commits into from
May 7, 2021

Conversation

IvanIsCoding
Copy link
Collaborator

@IvanIsCoding IvanIsCoding commented May 7, 2021

Format Python code with black to align code formatting with recent changes to other Qiskit repositories.

This formatting changes a massive amount of test files, so I will highlight the files that actually need to be reviewed:

  • tox.ini for adding tox -eblack and making flake8 agree with black
  • pyproject.toml for adding black settings
  • CONTRIBUTING.md with instructions to run black for new contributors
  • .github/workflows/main.yml for checking black codestyle in CI

@coveralls
Copy link

coveralls commented May 7, 2021

Pull Request Test Coverage Report for Build 820926823

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 96.578%

Totals Coverage Status
Change from base Build 810011546: 0.02%
Covered Lines: 6942
Relevant Lines: 7188

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Overall this LGTM. Personally I've resisted using black here because I personally really don't like some of it's style choices it makes. But, I think we've already (for quite some time) reached the point where retworkx is no longer solely my domain. With that in mind I think using black to handle the code formatting makes sense since it simplifies the code formatting for contribution in the same way rustfmt does (also the style is a closer match to rustfmt).

I just left a couple of comments inline. Of those comments most are just idle thoughts or questions for the future (mostly around line length) the only required changes are in the tox.ini.
Aside from the inline comments though, you will also need to manually update the lint github actions job. We don't use tox for that job because I wanted to run cargo directly for the well formatted compiler error message and we already build twice, so doing it again for tox seemed unnecessary. I guess you could update the job either to just explicitly run black --check --diff or change the lint jobs to be tox --skip-pkg-install -elint either would work (the later might be better to be more consistent between local and ci, especially with a pinned black version).

Comment on lines +118 to +137
self.assertEqual(
[
(
"a",
[
{"numeral": 9},
{"numeral": 8},
{"numeral": 7},
{"numeral": 6},
{"numeral": 5},
{"numeral": 4},
{"numeral": 3},
{"numeral": 2},
{"numeral": 1},
{"numeral": 0},
],
)
],
res,
)
Copy link
Member

Choose a reason for hiding this comment

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

Wow this is pretty ugly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps the beauty is that we didn't have to format it ourselves!

@IvanIsCoding
Copy link
Collaborator Author

IvanIsCoding commented May 7, 2021

I have addressed some comments and:

  • black version is now fixed
  • tox -elint and tox -eblack share the same environment
  • black is executed in CI. I opted to add a new command because I found it simpler
  • Line lenght is now 80

With regards to line lenght: I agree with the idea of using the same line lenght for cargo fmt and black, consistency is a great idea!

It is easier though to set black to use line lenght of 80 than to set cargo fmt to use line length of 100. We have way more Rust code than Python code, so I tried to be mindful and minimize the diff. Also, most of our Rust code is core functions while most of our Python code is test cases. So let's keep Rust as it is 😃

@mtreinish mtreinish merged commit 8efe47c into Qiskit:main May 7, 2021
@IvanIsCoding IvanIsCoding deleted the add_black branch May 8, 2021 01:40
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.

3 participants