Skip to content

Remove forced config file default of .coveragerc #508

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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?

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

Successfully merging this pull request may close these issues.

4 participants