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

feat: Add support for -x/--exclude-lines. #58

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DanCardin
Copy link

@DanCardin DanCardin commented Oct 19, 2024

Attempts to fix #26

I integrated the regex matching into the line-inclusion loop (Thus it works by excluding excluded lines from the total set), but it occurred to me afterwards, that it could perhaps instead be integrated similarly to _add_unseen_source_files. There, you're already scanning a bunch of files, compiling them, and adding their lines. It could work in reverse, where it scans all the files, compiles them, and adds matching lines to the "seen" set.

I'm not familiar enough with the code to know if one would make more or less sense than the other, but I think the way I have it implemented now is at least intelligible and seems to be performant.

I tried to defer imports to expensive modules (like re/inspect) up to the point of the exclude_lines feature being utilized. I dont see any slowdown when not making use of the feature, but a ~0.2s slowdown when making use of the feature on a ~6s test run (~10k LOC, since I figure it scales worse with numbers of code objects scanned).


I wasn't able to get all the tests running locally, it seems unrelated to my changes, and that it might be some environment specific thing that I'm not sure what I need to do; but I can't say for certain.


Also, I dont love this feature being purely given by command line options. if you're into this feature, i would ideally like to follow it up by (even optional) reading from pyproject.toml for a tool.slipcover setting (for this at a minimum)


frame = frame.f_back # type: ignore[assignment]
@functools.lru_cache(None)
Copy link
Author

@DanCardin DanCardin Oct 19, 2024

Choose a reason for hiding this comment

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

without this cache, I saw a ~2s slowdown relative to the current impl.

5.9s without exclusions
6.1s with exclusions and cache
8.8s with exclusions no cache
10.5s with coverage

Copy link
Collaborator

@jaltmayerpizzorno jaltmayerpizzorno left a comment

Choose a reason for hiding this comment

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

Hi, thank you for your pull request! Please check out my questions and/or make the changes requested.

src/slipcover/slipcover.py Outdated Show resolved Hide resolved
src/slipcover/slipcover.py Outdated Show resolved Hide resolved
src/slipcover/slipcover.py Outdated Show resolved Hide resolved
src/slipcover/slipcover.py Outdated Show resolved Hide resolved
src/slipcover/slipcover.py Outdated Show resolved Hide resolved
src/slipcover/slipcover.py Show resolved Hide resolved
@DanCardin
Copy link
Author

DanCardin commented Oct 23, 2024

Sorry, I think my original commit must have had some editor auto-formatting or something going on. I definitely was not consciously going through and removing annotations and altering those version-constrained blocks! Updated to be (hopefully now) only the actual changes related to the enhancement.

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.

Support coverage's exclude_lines setting
2 participants