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 formatter #444

Merged

Conversation

Spacetown
Copy link
Member

  • Move configuration for coverage from setup.cfg to pyproject.toml.
  • Switch from flake8 to flake9 (Fork which supports pytoml.xml) and update documentation.
  • Add configuration for black to format code.
    • Pro:
      • I used it in another project and the formatted code looks good for me.
      • Formatting of code is independand of the developer and looks the same in the whole project.
      • No need of configuration.
  • Contra:
    • No configuration possible.

@Spacetown Spacetown marked this pull request as draft December 11, 2020 20:07
@codecov
Copy link

codecov bot commented Dec 11, 2020

Codecov Report

Merging #444 (a4b5697) into master (136b154) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #444   +/-   ##
=======================================
  Coverage   95.52%   95.52%           
=======================================
  Files          20       20           
  Lines        2458     2458           
  Branches      420      420           
=======================================
  Hits         2348     2348           
  Misses         48       48           
  Partials       62       62           
Flag Coverage Δ
ubuntu-18.04 95.03% <ø> (ø)
windows-2019 95.11% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 136b154...a4b5697. Read the comment docs.

@Spacetown Spacetown force-pushed the add_pyproject_toml_to_configure_tools branch from 5b3977b to e98222d Compare December 11, 2020 20:22
@Spacetown Spacetown marked this pull request as ready for review December 11, 2020 20:34
@latk
Copy link
Member

latk commented Dec 11, 2020

After thinking about this for a bit, I'm not quite sure this is a step in the right direction.

  • creating a pyproject.toml? Probably fine, though I don't see the value yet. Yes, Toml is a better format than Ini, but just moving configuration around doesn't necessarily improve much.
  • using flake9? That looks like an incompatible fork that could cause problems if a first-time contributor wants to work on gcovr without a virtualenv, and who already has flake8 installed. If we want to move flake8 configuration into a config file, then setup.cfg would seem like the appropriate target.
  • the black formatter is totally fine, as long as it won't change the current code (otherwise, a commit that re-formats the code would be appropriate). If this tool is supposed to be part of the development workflow, then adding it as a dependency would be appropriate.
  • I don't quite see why requirements files are a problem. They are widely supported in the PIP ecosystem and easy to use. Our dependency lists don't have to be generated by code.

Of course I'm willing to be convinced otherwise. There are two very good aspects in this PR that I'd like to keep though: specifying a code formatter, and simplifying invocation of flake8 by using a config file.

@Spacetown
Copy link
Member Author

  • creating a pyproject.toml? Probably fine, though I don't see the value yet. Yes, Toml is a better format than Ini, but just moving configuration around doesn't necessarily improve much.

  • using flake9? That looks like an incompatible fork that could cause problems if a first-time contributor wants to work on gcovr without a virtualenv, and who already has flake8 installed. If we want to move flake8 configuration into a config file, then setup.cfg would seem like the appropriate target.

Flake9 only because flake8 doesn't support pyproject.toml. I thought one configuration to configure all tools would be fine. I think it will be better to document onla that flake8 is used and the options are fixed in a configuration (maybe setup.py).

  • the black formatter is totally fine, as long as it won't change the current code (otherwise, a commit that re-formats the code would be appropriate). If this tool is supposed to be part of the development workflow, then adding it as a dependency would be appropriate.

If we start to use black it should be added to the tests and a reformat of all files is needed one time.

* I don't quite see why requirements files are a problem. They are widely supported in the PIP ecosystem and easy to use. Our dependency lists don't have to be generated by code.

The runtime requirements are configured in setup.py, the development requirements in requirements.txt and the documentation requirements in doc/requirements.txt. I think, a single place to specify all requirements is better.

Of course I'm willing to be convinced otherwise. There are two very good aspects in this PR that I'd like to keep though: specifying a code formatter, and simplifying invocation of flake8 by using a config file.

The simplifying of the invocation is a big advantage.

@Spacetown Spacetown force-pushed the add_pyproject_toml_to_configure_tools branch 2 times, most recently from 1af8c04 to 6c4f6ea Compare January 7, 2021 18:44
@Spacetown Spacetown force-pushed the add_pyproject_toml_to_configure_tools branch 6 times, most recently from f756b0a to 13a44a8 Compare February 1, 2021 21:10
@Spacetown
Copy link
Member Author

@latk I've revert the pyproject.toml, moved the configuration of flake8 to setup.cfg and introduced black.
The errorlevel of black is ignored at the moment because I don't want to reformat all the source files. This can be done if the file is changed.

Switch from flake8 to flake9 (Fork which supports pytoml.xml).
Move configuration for coverage from setup.cfg to pyproject.toml.
@Spacetown Spacetown force-pushed the add_pyproject_toml_to_configure_tools branch from 13a44a8 to 2f12740 Compare February 1, 2021 21:41
Copy link
Member

@latk latk left a comment

Choose a reason for hiding this comment

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

Looks good! Just two minor inline comments below.

For future reference, updating the title might also be a good idea.

Unfortunately, reformatting all code will lead to substantial merge conflicts. So we should probably delay that step for a bit until fewer PRs are still pending. So it's good that the CI failure is ignored (for now).

.dockerignore Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
Co-authored-by: Lukas Atkinson <opensource@LukasAtkinson.de>
@Spacetown Spacetown changed the title Add pyproject.toml to configure tools. Add black formatter Feb 2, 2021
@Spacetown Spacetown added this to the 4.3 milestone Feb 2, 2021
@Spacetown Spacetown merged commit 7d1babb into gcovr:master Feb 2, 2021
@Spacetown Spacetown deleted the add_pyproject_toml_to_configure_tools branch February 2, 2021 05:50
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

2 participants