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

Make it possible to skip package coverage #97

Merged
merged 4 commits into from
Feb 3, 2025
Merged

Conversation

punxaphil
Copy link
Contributor

Sometimes the package listing can become really verbose. This change allows the user of the action to only show overall coverage. Example:

image

@tm1000
Copy link
Member

tm1000 commented Jan 24, 2025

Doesn't only_list_changed_files do effectively the same thing?

@punxaphil
Copy link
Contributor Author

punxaphil commented Jan 24, 2025

Haven't tried it actually 😀
What if you have a pr with 100 files, that's gonna be quite verbose i guess?

@tm1000
Copy link
Member

tm1000 commented Jan 24, 2025

@punxaphil I see your point there :-)

You did a lot of changes here so I just need to properly walk through them all if you dont mind. But thank you for the work and it will be merged

tm1000
tm1000 previously approved these changes Jan 24, 2025
Comment on lines 90 to 91
let stdoutWriteCalls = getStdoutWriteCalls()
expect(stdoutWriteCalls).toMatchSnapshot()
Copy link
Member

Choose a reason for hiding this comment

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

Im assuming this was just test broiler plating which I dont think need to necessarily change but just checking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. It was for debugging. Reverted!

Comment on lines 82 to 83
let stdoutWriteCalls1 = getStdoutWriteCalls()
expect(stdoutWriteCalls1).toMatchSnapshot()
Copy link
Member

Choose a reason for hiding this comment

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

Im assuming this was just test broiler plating which I dont think need to necessarily change but just checking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. It was for debugging. Reverted!

@tm1000
Copy link
Member

tm1000 commented Jan 24, 2025

Actually looks rather good to me. Ignore the failed CI its because it sometimes has an issue commenting on PRs made from external contributors which I thought should be fixed as a result of this: #64

@punxaphil punxaphil requested a review from tm1000 January 26, 2025 13:59
@punxaphil
Copy link
Contributor Author

@tm1000 Thanks for reviewing! Are we ready to merge, or any other changes needed from me?

@tm1000 tm1000 merged commit 9a4ab37 into clearlyip:main Feb 3, 2025
1 check failed
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.

2 participants