-
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
Fix pyproject.toml support #16
Conversation
Signed-off-by: Hiroshi Miura <[email protected]>
Will this be merged soon? |
It's not completely clear to me what's the problem we're trying to solve here to be honest. |
@AndreMiras The problem for me is that all my configuration is in pyproject.toml and the action fails. It looks like it is because it is unable to load that configuration. Maybe it doesn't need to, but uses the coverage dependency which tries to - I don't know. See: #20. It would be good if you could see how to make a setup with the action for a project with no tox.ini, .coveragerc etc., only a pyproject.toml with coverage settings in it. |
I have the same problem. Here us a full traceback.
|
In the end I gave up as I couldn't find a workaround and switched to codecov. |
@AndreMiras coveralls uses coverage under the hood like you can see in the traceback. |
Signed-off-by: Hiroshi Miura <[email protected]>
Signed-off-by: Hiroshi Miura <[email protected]>
70ae6ad
to
df12392
Compare
I pushed modification for Dockerfile that fixes the issue. |
I'm getting the same traceback as @loicgasser. Switching to @miurahr's repository in my GH actions fixes the issue for me. |
What is the status of this PR? I'm running into this with my projects as I switch to pyproject.toml for all my configuration (over setup.cfg or other). |
Waiting @AndreMiras to see things under the hood clearly.
|
Is there any update on this? Looks like just a missing dependency issue, coveralls depends on coverage, coverage raises if it sees This actions is installing coveralls which in turns installs coverage, just adding toml to the requirements should fix it. |
This reverts commit df12392.
adjust comment on #20 |
|
||
[tool.coverage.report] | ||
show_missing = true | ||
exclude_lines = ["if __name__ == .__main__.:", "pragma: no-cover", "@abstract", "def __repr__"] |
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.
Is this line needed for pyproject.toml support? What is it doing? What about the other changes to this file? Is build-system
needed?
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.
build-system
section is alternative of setup
of setup.py
. When there is no setup.py
, and when using setuptools
for build, it is to be a same configuraiton.
A file pyproject.toml
is used for test, not to supporting pyproject.toml
in users project.
These lines are adeed for testing purpose.
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.
Testing of the pyproject.toml support? I think the extra changes in this PR were a little confusing otherwise so it was more difficult to review by the maintainer.
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.
It is mandatory to test. It is new standard in python.
https://packaging.python.org/en/latest/tutorials/packaging-projects/#creating-pyproject-toml
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.
I'm well aware of what pyproject.toml is and its purpose. I was only trying to say that it isn't clearly mentioned in any comments or in the PR description what the extra configuration in this pyproject.toml is doing. The coverage
configuration is different from the existing configuration in setup.cfg
(which is not removed in this PR). You also define a build system where previously there wasn't one so it isn't clear why one is needed.
I'm also a little confused because the tests in push.yml
and in make tests
don't explicitly handle pyproject.toml
so by putting it here in the root of the repository I assume all tests are now using this pyproject.toml
. However, that doesn't test the absence of a pyproject.toml
(by using the existing setup.cfg). Shouldn't both cases be tested? Perhaps with a subdirectory for different test configuration cases? One with a setup.cfg and one with a pyproject.toml?
pyproject.toml is the new unified Python project settings file that (is supposed to) replace setup.py. pyproject.toml is especially useful to avoid having a config file per each python tool used in your python app. E.g. I have a pyproject.toml hosting the config of black, iosort, coverage and cibuildwheel tools. As such I would deem this feature as a must-have. |
I want to chime in here to echo that there is elevated need to support coverage configurations in pyproject.toml on this action. This standard has been adopted by PEP for several (5+) years now. I'm not sure why this hasn't been addressed to date. |
I want to add again that my PR #24 includes the actual |
Thanks, I'll try to look this up over the weekend |
Hi Andre, thanks for this really nice work. We are still waiting for this fix :).
|
For those who seek a temporary workaround: |
@AndreMiras I ran into this in another project. Anything we can do to help? As a reminder, my PR #24 adds pyproject support and Cython. |
Merged thanks, let me know how it goes with it |
I just did a quick update to switch from my PR's branch to the develop branch here and it looks like it is all working. Thanks @AndreMiras. I think this can be closed. |
Thanks for the follow up, closing then |
README express toml support. Because actions does not install toml extension so it always fails.
This PR add pyproject.toml and CI will test with the file.
Signed-off-by: Hiroshi Miura [email protected]