feat: Add option to choose mode for display file coverage #192
Conversation
There was a problem hiding this comment.
Hi @sfer23, first of all thank you very much for opening this PR 🙇🏻.
It looks really great already! 👍🏻
There are a few typing errors and some areas that require a little bit of brushing up, so I left some comments to address them, and with suggestions where it makes sense.
Option mixed
As written in one of them, I think an option of mixed is not really required. I like the all mode (first showing the changes, then all other files) either way, so I'd make it the default. I even thought about making changes the default behavior, but I am a bit inconclusive as it would be a breaking change.
Missing: Make table conditional if mode is set to none or no changes
What's still missing in this PR, but I think would be important is to not generate / show the coverage table if the Mode is set to none or if it is set to changes - but there are none (e.g. if only untested areas of the code were changed or of it is not in the context of a PR). I guess the right place to address this would be in the index.js. Would you mind putting that in?
Hand over?
Last but not least: I really, really appreciate the work you already put into this and don't want to take up more of the time if you don't want to. So if you want me to take over, just let me know and I'll happily do the remaining work. Of course, I'll squash it all into your commit in the end, so you'll get the contribution any ways 🙂.
Of course, if you want to take it to the finish line, I 💯% will let you do it. 🙂
|
Thanks for feedback, it's perfect, I will fix that, but I can take some time (estimate about next week) |
|
Hey @sfer23 , just quickly wanted to check in if you were able to work on this or need any support? Again, if you want me to take over, no problem. |
|
@davelosert Thanks for reminder, it's really not enough hours in days :) Here is a few fixes, not sure about one moment with Octkokit, my IDE say it's will not work (because of plugins) so I keep it as it was. But anyway I think you have much more experience with Node and GitHub so feel free to fix that moment also. I also removed 'mixed' mode, because I added it just for fully backward compatibility and if you also think that it's not needed - I removed it. Here is nice repo where I take a look for some examples: getsentry/paths-filter It's really nice to have such kind of code review and I thankful for your time also to help me, Thanks. |
|
@sfer23: Great work, and I'll gladly finish up here! Just a minor thing: Update: Nevermind my request above for write access to your fork - I already got it, my Codespace just seems to have been in a weird state so I couldn't push. I was able to do so now! 🙂 |
|
@sfer23 : Thank you so much again for your work here. 🙂 I will merge this in tomorrow and do some testing before releasing it, hopefully also tomorrow. |
BREAKING CHANGE: The File-Coverage will now only report files that were actually changed in the pull-request on default. This is to avoid large projects having problems with the comments on the PR being too big. To get the old behavior (always report all tested files), use the new setting 'file-coverage-mode: all'. --------- Co-authored-by: David Losert <davelosert@github.com>
# [2.0.0](v1.4.0...v2.0.0) (2023-06-13) ### Bug Fixes * Fixes broken file-coverage links ([#230](#230)) ([25ca77f](25ca77f)) ### Features * Add option to choose mode for display file coverage ([#192](#192)) ([148463f](148463f)) ### BREAKING CHANGES * The File-Coverage will now only report files that were actually changed in the pull-request on default. This is to avoid large projects having problems with the comments on the PR being too big. To get the old behavior (always report all tested files), use the new setting 'file-coverage-mode: all'.
Fixes #160, #190