Skip to content

Docs and tests#8

Open
blaggacao wants to merge 3 commits intoknowark:masterfrom
xoe-labs:docs-and-tests
Open

Docs and tests#8
blaggacao wants to merge 3 commits intoknowark:masterfrom
xoe-labs:docs-and-tests

Conversation

@blaggacao
Copy link
Contributor

@blaggacao blaggacao commented Dec 6, 2018

Just some small fixes which should streamling tox testing and general development.

I included pre-commit hooks, please have a look at the respective commit message for detailed information look at the .pre-commit-config.yaml.

I will add one extra commit which will transform styling according to black, if that's too much for your taste just ignore the pre-commit parts. However I found them useful in other projects...

@blaggacao
Copy link
Contributor Author

blaggacao commented Dec 6, 2018

@tebanep Don't be scared by the huge changeset of 2520495 it's fully automatically done by the different pre-commit hooks (notably black) and won't break anything with 99% security.

Here is the output of flake8 and bugbear that seem could need some attention:

Flake8...................................................................Failed
hookid: flake8

facturark/client/client.py:16:72: B006 Do not use mutable data structures for argument defaults. All calls reuse one instance of that data structure, persisting changes between them.
facturark/signer/signer.py:64:95: B950 line too long (94 > 80 characters)
facturark/signer/signer.py:102:9: F841 local variable 'signed_document_element' is assigned to but never used
tests/imager/test_imager.py:9:5: F841 local variable 'message' is assigned to but never used
tests/signer/test_verifier.py:43:9: F841 local variable 'result' is assigned to but never used
tests/test_main.py:100:5: F841 local variable 'result' is assigned to but never used
tests/test_main.py:181:63: F821 undefined name 'mock_build_document'
tests/test_main.py:193:5: F821 undefined name 'cli_build_invoice'
tests/test_utils.py:56:9: F841 local variable 'child' is assigned to but never used
tests/test_utils.py:62:5: F841 local variable 'text' is assigned to but never used
tests/test_utils.py:74:9: F841 local variable 'child' is assigned to but never used

@blaggacao
Copy link
Contributor Author

blaggacao commented Dec 6, 2018

Tests are failing (as expected and reproduced locally) on py27, but I think they never did actually work...

Maybe you can use pasterurize from python-future to provide the required compatibility layer, massively and automatically.

@blaggacao
Copy link
Contributor Author

@tebanep I'm getting excited 👍

@tebanep
Copy link
Contributor

tebanep commented Dec 7, 2018

Tests are failing (as expected and reproduced locally) on py27, but I think they never did actually work...

Maybe you can use pasterurize from python-future to provide the required compatibility layer, massively and automatically.

We'd rather handle compatibility manually, as we are aiming to support python2 just for the next year. It will be history after that, anyway. :)

@blaggacao
Copy link
Contributor Author

blaggacao commented Dec 8, 2018

I see.

Out of the series of commits in here just let me know which one are interesting to you and I'll rebase and prepare a proper PR to your liking.

This was referenced Dec 12, 2018
@blaggacao
Copy link
Contributor Author

If you tell me which of those commits are to your liking, I'll just do a quick cleanup and rebase properly and cleanup the other PRs ...

@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #8 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #8   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          94     94           
  Lines        1954   1954           
  Branches       96     96           
=====================================
  Hits         1954   1954

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 a0b3068...72aa630. Read the comment docs.

@blaggacao
Copy link
Contributor Author

@tebanep I reduced the diff to the minimum sensible proposal.

  • No p27 fixing
  • only include pre-commit hooks
  • add flake8 config file

This way, you can run pre-commit run --all at any time you want (and where it makes sense to a clean history). Accepting this PR, going forward hooks are only executed on file diffs (and only if locally initialized)

@tebanep
Copy link
Contributor

tebanep commented Dec 12, 2018

@tebanep I reduced the diff to the minimum sensible proposal.

* No p27 fixing

* only include pre-commit hooks

* add flake8 config file

This way, you can run pre-commit run --all at any time you want (and where it makes sense to a clean history). Accepting this PR, going forward hooks are only executed on file diffs (and only if locally initialized)

@blaggacao Thanks a lot for your commitment towards this project! Give me until the weekend to review your proposal in calm as I think I need a little bit more of background. 👍

David Arnold added 3 commits December 13, 2018 15:39
Install with `pip install pre-commit`
Activate with `pre-commit` in the root of the work dir
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants