-
Notifications
You must be signed in to change notification settings - Fork 19
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 Cython and toml dependencies to support Cython coverage plugin #24
Add Cython and toml dependencies to support Cython coverage plugin #24
Conversation
Signed-off-by: Hiroshi Miura <[email protected]>
Signed-off-by: Hiroshi Miura <[email protected]>
Signed-off-by: Hiroshi Miura <[email protected]>
…re-cython-coverage
Thanks, I'll eventually have time this end of week to look it up |
pyproject.toml
Outdated
[build-system] | ||
requires = ["setuptools>=42", "wheel"] | ||
build-backend = "setuptools.build_meta" | ||
|
||
[tool.coverage.paths] | ||
source = ["src", "*/site-packages"] | ||
|
||
[tool.coverage.run] | ||
branch = true | ||
relative_files = true | ||
|
||
[tool.coverage.report] | ||
show_missing = true | ||
exclude_lines = ["if __name__ == .__main__.:", "pragma: no-cover", "@abstract", "def __repr__"] | ||
|
||
[tool.black] | ||
line-length = 88 | ||
target-version = ['py38'] | ||
|
||
[tool.isort] | ||
multi_line_output = 3 | ||
include_trailing_comma = true | ||
force_grid_wrap = 0 | ||
use_parentheses = true | ||
line_length = 88 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djhoese It's not clear to me why we need all these?
Can we keep the PR minimal to cover only the Cython case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndreMiras From fwhat I can tell it looks like @miurahr tried to move some/all of the configuration in setup.cfg while at the same time adding a build-system
section so this github action could be built as a python package maybe? It also looks like they added default configurations for black and how coverage for this project itself is reported. None of this requires that black is run, but the coverage configuration might need to be up to you if you agree with the lines that are excluded.
If you'd like I can revert all of the pyproject.toml changes that aren't needed and see how things behave.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I saw some of the config were indeed just moved around, but other got added and I'm not too sure about it, like exclude_lines
indeed.
Could you PR only your changes if that's enough to cover your case?
Maybe you can amend the commits to recycle this PR if you know how to.
We don't need the whole back and forth history, so it could look like:
git reset --hard upstream/develop
git cherry-pick f3f66e3
git commit --amend
git push -f origin feature-cython-coverage
Or simply close and open an new one.
I leave that to you
Ok so I was going to revert the commits, but I really wanted to include @miurahr as an author in this PR somewhere since the original changes were theirs. They've also modified their PR to not remove configuration from the setup.cfg file. However, I don't see a reason to have both a pyproject.toml and a setup.cfg. There is nothing in the CI or tests that say "run this action and point to specifically config file X". This type of logic and testing should be done in a separate PR in my opinion. This PR is now as simple as I think it can be. One downside to merging this is that it may close @miurahr's PR even though it doesn't do any of the pyproject.toml changes anymore. If you don't want all these commits in the main branch you could also squash and merge this PR (which would also leave @miurahr's PR open). @AndreMiras what do you think? |
@AndreMiras Any chance you could merge this? Or would you be willing to add some maintainers to the project? |
Squashed & merged that eventually, thanks for the follow ups 🙏 |
Closes #22
The Cython.Coverage plugin requires that Cython be installed. Even though this action is not run when the coverage is generated the coverage configuration file is still loaded (setup.cfg or coveragerc or pyproject.toml) with coveralls. When plugins are specified this means it tries to import them. Since the
Dockerfile
image used by this action does not contain Cython this import fails (as described in #22). This PR adds Cython as a dependency to the Dockerfile so that this doesn't fail.Here is an example of my pyproject.toml where I have coverage configured:
Note this includes the fixes/changes from #16 since I'm using pyproject.toml.
CC @miurahr