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

Remove forced config file default of .coveragerc #508

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ofek
Copy link

@ofek ofek commented Dec 3, 2021

There has been support for pyproject.toml ever since 5.0 nedbat/coveragepy#664 (comment) and based on https://coverage.readthedocs.io/en/6.2/config.html it will automatically find the right file (logic here: https://github.com/nedbat/coveragepy/blob/6.2/coverage/config.py#L493-L519)

I tested this locally and now config can properly be read from the other fallback files

@ofek
Copy link
Author

ofek commented Dec 12, 2021

@ionelmc @graingert Hello! When you get a moment can you please take a look at this? I can't figure out why the failures are occurring.

@ionelmc
Copy link
Member

ionelmc commented Dec 21, 2021

Ooof sorry for delay, CI approved now. At first glance it seem fine. Please also update AUTHORS/CHANGELOG.rst

@ofek
Copy link
Author

ofek commented Dec 21, 2021

Done! I don't know how to fix the tests though.

@ofek
Copy link
Author

ofek commented Mar 30, 2022

Can I please get help with this?

@ionelmc
Copy link
Member

ionelmc commented Jul 22, 2022

Hello, sorry for tardy response - can you rebase on the master? The CI was cleaned up.

@ofek
Copy link
Author

ofek commented Jul 22, 2022

Done!

@ofek
Copy link
Author

ofek commented Jul 23, 2022

I'm not sure how to fix

@ofek
Copy link
Author

ofek commented Aug 24, 2022

I still can't figure it out

@@ -98,7 +102,7 @@ def set_env(self):
if os.path.exists(config_file):
os.environ['COV_CORE_CONFIG'] = config_file
else:
os.environ['COV_CORE_CONFIG'] = os.pathsep
os.environ['COV_CORE_CONFIG'] = ''
Copy link
Member

Choose a reason for hiding this comment

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

I believe this change is the cause of all the regressions.

Note that we have this in embed.py and you need to keep it working:

        if cov_config == os.pathsep:
            cov_config = True

@@ -70,6 +70,10 @@ def __init__(self, cov_source, cov_report, cov_config, cov_append, cov_branch, c
self.topdir = os.getcwd()
self.is_collocated = None

# If unset (the default), indicate fallback behavior for other files like pyproject.toml.
# See https://github.com/nedbat/coveragepy/blob/6.2/coverage/control.py#L144-L146
self.config_file_choice = self.cov_config or True
Copy link
Member

Choose a reason for hiding this comment

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

Also I'd simply rename this to config_file. Maybe it's just me but a dropdrown widget pops up in my head when I see "choice" - too much Django I guess xD

@ofek
Copy link
Author

ofek commented Nov 27, 2022

Can you please help?

@webknjaz
Copy link
Member

@ofek this looks old and needs conflict resolution. Try rebasing once more?

@ofek
Copy link
Author

ofek commented Mar 18, 2024

CI was failing and I don't think the maintainer has time for this unfortunately so there's no point. If you really want me to I will but I feel like it's a waste of time.

@ionelmc
Copy link
Member

ionelmc commented Mar 19, 2024

Sorry, I know this feels bad, having to rebase over and over. But I did point in code commends at the potential cause of the regressions. In hindsight I should have asked you to describe more clearly what this PR is supposed to do and help figure out a good solution instead of reviewing a solution that I am not sure what is supposed to fix. Maybe that would had gotten things moving faster.

Can we restart this? Is there a problem with pytest-cov loading the coverage configuration from a pyproject.toml?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants