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

blacken code and clean up test-env install, introduce pre-commit #16

Merged
merged 1 commit into from Jun 3, 2020

Conversation

hairmare
Copy link
Member

@hairmare hairmare commented Oct 14, 2019

This PR reformats the code using black, configures isort & flake8 to match and adds pre-commit to the README as well as to the travis tests. I also took care of fixing or ignoring any issues found by flake8.

My apologies for the huge changeset, all of this does not touch/refactor any of the business logic or logic in setup.py (it touches how requirements are read from txt files in setup.py)... The changes to .travis.yml are kept as minimal as possible.

I plan on taking care of the werkzeug deprecation warnings in a future pull request and this prepares the repo to make it easy to do so. I'll also automate dependency bumping PRs once this is merged.

@hairmare hairmare requested a review from smlz October 14, 2019 18:40
@codecov-io
Copy link

codecov-io commented Oct 14, 2019

Codecov Report

Merging #16 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #16   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         481    481           
  Branches      109    109           
=====================================
  Hits          481    481
Impacted Files Coverage Δ
klangbecken_api.py 100% <100%> (ø) ⬆️

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 05c456a...35f3aa6. Read the comment docs.

@hairmare hairmare force-pushed the chore/cleanup-flake8-and-tests branch 3 times, most recently from 35f3aa6 to 9f621b8 Compare November 15, 2019 12:08
@smlz
Copy link
Member

smlz commented Dec 12, 2019

Hey Bicku

Finally things calmed down a bit, an I get to review your pull request.

This is really cool! Thanks a lot. Can you rebase the changes once more, with the current version of the project?

Also, I'd like to try black's --skip-string-normalization option, as this might significantly reduce the number of changes.

The deprecations are mostly gone by now, as authentication is now done with jwt-tokens instead of session cookies.

Copy link
Member

@smlz smlz left a comment

Choose a reason for hiding this comment

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

Try to move as much as possible to setup.cfg and reduce the number of dependencies to a minimum.

.flake8 Outdated Show resolved Hide resolved
.isort.cfg Outdated Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
- id: black
name: black
language: system
entry: black
Copy link
Member

Choose a reason for hiding this comment

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

Can we add the --skip-string-normalization option here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the ' more than the " but I hate having to switch to what each project wants even more. If we skip string normalization we'll have a mix of " and ' pretty soon.

requirements-dev.txt Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
requirements-dev.txt Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@hairmare hairmare changed the title blacken code and clean up test-env install, introduce pre-commit WIP: blacken code and clean up test-env install, introduce pre-commit Apr 29, 2020
@hairmare hairmare force-pushed the chore/cleanup-flake8-and-tests branch 8 times, most recently from fe01d84 to 9ef59cc Compare June 3, 2020 08:49
@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2020

Codecov Report

Merging #16 into master will not change coverage.
The diff coverage is 72.46%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #16   +/-   ##
=======================================
  Coverage   78.90%   78.90%           
=======================================
  Files           1        1           
  Lines         602      602           
  Branches      141      141           
=======================================
  Hits          475      475           
  Misses        110      110           
  Partials       17       17           
Impacted Files Coverage Δ
klangbecken.py 78.90% <72.46%> (ø)

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 fbee74c...b005ee7. Read the comment docs.

@hairmare hairmare force-pushed the chore/cleanup-flake8-and-tests branch 2 times, most recently from 1732f3a to d0f5f66 Compare June 3, 2020 09:46
@hairmare hairmare force-pushed the chore/cleanup-flake8-and-tests branch from d0f5f66 to b005ee7 Compare June 3, 2020 09:47
@hairmare hairmare changed the title WIP: blacken code and clean up test-env install, introduce pre-commit blacken code and clean up test-env install, introduce pre-commit Jun 3, 2020
@hairmare
Copy link
Member Author

hairmare commented Jun 3, 2020

I addressed all the feedbacks except the one about --skip-string-normalization. I believe strongly that this needs to be standardized, in fact string normalization is one of the reasons I started this in the first place (after always finding myself using " and then changing it to ' to match with the remaining code). I would have been easier on everyone's eyes if we could have stuck with ' but this is mostly about delegating such mundane decisions to black, so " it is.

@spameier
Copy link
Member

spameier commented Jun 3, 2020

I addressed all the feedbacks except the one about --skip-string-normalization. I believe strongly that this needs to be standardized, in fact string normalization is one of the reasons I started this in the first place (after always finding myself using " and then changing it to ' to match with the remaining code). I would have been easier on everyone's eyes if we could have stuck with ' but this is mostly about delegating such mundane decisions to black, so " it is.

I agree. After all that's why we want to use black.

@hairmare hairmare requested a review from spameier June 3, 2020 11:22
@hairmare hairmare merged commit b94daa0 into radiorabe:master Jun 3, 2020
@hairmare hairmare deleted the chore/cleanup-flake8-and-tests branch June 3, 2020 13:34
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

5 participants