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 ability to load configuration from tox.ini #86

Merged
merged 8 commits into from
Nov 13, 2018
Merged

Add ability to load configuration from tox.ini #86

merged 8 commits into from
Nov 13, 2018

Conversation

GhostofGoes
Copy link
Contributor

Addresses issue #83

Copy link
Owner

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

Looks good to me, with minor issues:

  • Chocolatey is currently down, which breaks Appveyor builds
  • The new docstring doesn't follow PEP-257
  • I'm sure flake8 will complain about those backslashes and excessive indentation for return True inside the new if.
  • I don't see a commit status from coveralls so I'll have to check test coverage manually.

I'll retry Appveyor builds when Chocolatey is up again. Please ping me if I forget. ;)

@GhostofGoes
Copy link
Contributor Author

Thanks for pointing those out, I forgot PEP-257 has a newline before the trailing closer 😅

Fixed the backslashes (tho flake 8 surprisingly didn't complain) as well as all the other issues flake8 had. However it's still warning about line breaks before/after binary operators. Not sure how else I could structure that logic across multiple lines without some if-break unpleasantry. Open to suggestions.

Also, for some reason the license classifier for PyPI was being dynamically determined at build time. I removed that, but can put it back if that was something you intended to keep.

Here are the remaining flake8 issues:

flake8 runtests: commands[0] | flake8 check_manifest.py tests.py setup.py
check_manifest.py:596:13: W503 line break before binary operator
check_manifest.py:613:13: W503 line break before binary operator
check_manifest.py:614:17: W503 line break before binary operator
check_manifest.py:728:35: W504 line break after binary operator
check_manifest.py:746:16: W504 line break after binary operator
check_manifest.py:764:13: W503 line break before binary operator

@GhostofGoes
Copy link
Contributor Author

Also, I ran the coverage checks locally, the portions I added (apparently) have coverage. Would be good if you double checked that though, since I'm still fairly new to actually using code coverage.

@mgedmin
Copy link
Owner

mgedmin commented Nov 8, 2018

Thanks for pointing those out, I forgot PEP-257 has a newline before the trailing closer 

I was also hinting that it requires a blank line after the first statement. ;)

Fixed the backslashes (tho flake 8 surprisingly didn't complain) as well as all the other issues flake8 had. However it's still warning about line breaks before/after binary operators.

Oh, that! It's a long story.

There was a PEP-8 update (it recommended line break after binary operators, and then was changed to recommend one before), a pycodestyle update, and then a flake8 update, and now flake8 complains in both cases (!?!?!) and requires you to add one of the two warnings to the ignore setting in the [flake8] section in setup.cfg.

Wait, actually I think this is probably just because I have an ignore setting, so it overrides whatever the default ignore list is.

Anyway, I'll fix it.

Also, for some reason the license classifier for PyPI was being dynamically determined at build time. I removed that, but can put it back if that was something you intended to keep.

I wanted there to be a single source of truth for the license information (__licence__ in check_manifest.py), so I overengineered it a little bit. Please put it back? I changed my mind, please don't put it back ;)

@mgedmin
Copy link
Owner

mgedmin commented Nov 8, 2018

Also, I ran the coverage checks locally, the portions I added (apparently) have coverage. Would be good if you double checked that though, since I'm still fairly new to actually using code coverage.

Coveralls shows the coverage (at 100%) if I go to https://coveralls.io/github/mgedmin/check-manifest and find the build of this PR. What I don't understand is why it doesn't update the commit status here, like it does for other repositories:
ekrano nuotrauka is 2018-11-08 14-54-38

Copy link
Owner

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

May I ask for a rebase on master, if it's not too much trouble? I had some unpushed commits in my local tree. There should be no conflicts.

check_manifest.py Outdated Show resolved Hide resolved
check_manifest.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@mgedmin
Copy link
Owner

mgedmin commented Nov 8, 2018

(I've unlinked my GitHub account from coveralls and relinked it back; let's see if it updates the commit status after your next push!)

@mgedmin
Copy link
Owner

mgedmin commented Nov 8, 2018

Meanwhile I've created a PR to add flake8 to the CI: #87

If I merge it first, it'll create some merge conflicts for you.

@GhostofGoes
Copy link
Contributor Author

I'm fine with rebasing.

Go ahead and merge that, I'll remove flake8 from this PR.

@GhostofGoes
Copy link
Contributor Author

Also, I've been running it with Python 3.7 in several projects without any issues, as well as on Windows. So I think you can safely say it supports 3.7 😄

@mgedmin
Copy link
Owner

mgedmin commented Nov 9, 2018

Also, I've been running it with Python 3.7 in several projects without any issues, as well as on Windows. So I think you can safely say it supports 3.7 

I thought I did?

Ah, I never made a new release with that commit.

@mgedmin
Copy link
Owner

mgedmin commented Nov 9, 2018

Merged, so now this PR has conflicts in setup.py.

@mgedmin
Copy link
Owner

mgedmin commented Nov 9, 2018

(Oh, would you mind adding a line about this change to CHANGES.rst?)

@GhostofGoes
Copy link
Contributor Author

Fixed conflicts and added a note to the changelog.

The only reason I mentioned 3.7 was the changelog stated "claim" 3.7, when we know it works on 3.7. Don't have to claim anything 😉

@mgedmin
Copy link
Owner

mgedmin commented Nov 13, 2018

The only reason I mentioned 3.7 was the changelog stated "claim" 3.7, when we know it works on 3.7. Don't have to claim anything wink

I wanted to distinguish "Add support for Python 3.7", which to me implies some code changes were needed to fix breakage on Python 37, from "Claim support for Python 3.7", where everything works fine, it was just not noted in the documentation/pypi classifiers.

It's a silly distinction. "Add support" could very well mean "from now on we'll have CI running tests for this Python version and we will pay attention and fix bugs if any crop up".

setup.py Outdated Show resolved Hide resolved
@mgedmin mgedmin merged commit bb184bf into mgedmin:master Nov 13, 2018
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