Linting #150
Linting #150
Conversation
|
@herrboyer let me know when you think this is ready and I will take a look. |
|
Hi @edsu sorry for the noise on this PR, I had some problems with Travis... I think I'm done with the linting, let me know what you think. About the last item on the checklist, would adding a few words in a CONTRIBUTING.md file be OK with you ? |
|
In fact I think that the information on the linter have more their place in the documentation ... The contribution guide is a little too vast for me to approach it on this PR ... Somehow I went overboard and did a reorganization of the documentation in the last commit (same content, but in a single page and a I think better ToC). Let me know @edsu if I should rollback. |
|
Well, now I think I'm done @edsu, looking forward to reading your feedback ! |
|
@herrboyer this is an incredible amount of cleanup, thank you for doing it! Adding a CONTRIBUTING.md file would be useful yes if you still have the energy for that. Were you thinking that's where details could go about how to lint? I hadn't seen black before, so I'll be interested to see how it works in practice for my other projects. |
|
Oh, I see the new Contributing section in the docs. Did you think that would work better as a separate file in the root? I worry that it might get a bit lost in the docs. And thanks to @Wooble I see that black is smart enough to fix the problems it finds :-) So maybe it wasn't so much work for you to do, but I still appreciate the time you spent adding it. |
.travis.yml
Outdated
| - stage: lint | ||
| name: "Check linting with flake8" | ||
| install: pip install -r requirements.dev.txt | ||
| script: find . -name \*.py -exec flake8 {} + |
There was a problem hiding this comment.
Could this be simplified as:
script: flake8 pymarc
There was a problem hiding this comment.
we also need to lint the test module, and maybe other python files in the project's root (like the apply_licence_header proposed by dan), but I agree this was too complex, I've committed this, would it be better ?
- stage: lint
name: "Check linting with flake8"
install: pip install -r requirements.dev.txt
script: flake8 .
- stage: lint
name: "Check format with black"
install: pip install -r requirements.dev.txt
script: black --quiet --diff .
.travis.yml
Outdated
| - stage: lint | ||
| name: "Check format with black" | ||
| install: pip install -r requirements.dev.txt | ||
| script: find . -name \*.py -exec black --quiet --diff {} + |
There was a problem hiding this comment.
Could this be simplified as:
script: black --quiet --diff pymarc
It doesn't look like black will exit non-zero, so when it runs across errors it will simply print out some diffs and the build will continue? Is this really serving any purpose?
There was a problem hiding this comment.
My mistake, forgot the --check flag, fixed by dddca14
Yes black was able to apply all the formatting rules, it's a wonderful tool. Most of my time was spent on making flake8 happy (doctrings, unused imports/variables, etc.) |
Well I hesitated a bit about this, but you're right, it's a bit lost after the api docs... Moved it to the standard CONTRIBUTING.md file here 6800d5d |
|
🙌 |
Fixes #148