-
Notifications
You must be signed in to change notification settings - Fork 32
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
Add SUBPROJECT_ID setting #213
Conversation
End-to-end public repo |
Coverage reportThe coverage rate went from
Diff Coverage details (click to unfold)coverage_comment/github.py
coverage_comment/settings.py
coverage_comment/main.py
coverage_comment/template.py
|
f4b49e5
to
d53fffc
Compare
Shall we ? |
Oh, I'm sorry. I didn't realise that this was already review/merge ready. I thought you were still working on it because you never requested a review and because in some places the necessary changes have not yet been made. For example, here and also at another place the used marker is not the formated, but still the old one (Or am I completely off track?). And I also noticed something else that I thought is still missing. So I'm sorry if you've been waiting for my review all this time. |
Oh, don't worry, no it's probably me :) I'll self-review and ping you when it's ready ! |
8cbb0b5
to
56f2ceb
Compare
I've re-read all and added tests. |
8ed74f3
to
c180879
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks great (:clap:). Very nice that we will soon be able to support monorepos with it.
I've tested it fairly extensively in a test repo with multiple IDs in PR and push mode. And the code seems to run smoothly as far as I've seen.
I still had a few minor comments and I took the liberty of fixing the e2e tests that were failing.
I think it might be a good idea if we could somehow integrate the ID into readme.md.j2
as well. Maybe even in the heading, so that the projects are easier to distinguish when looking at the README on the coverage data branch.
Otherwise, everything looks good 👍
/invite |
Co-authored-by: kieferro <81773954+kieferro@users.noreply.github.com>
for more information, see https://pre-commit.ci
Co-authored-by: kieferro <81773954+kieferro@users.noreply.github.com>
Good idea ! |
I think we're good 🎉 |
Yes, looks great. Since we're both happy with it, I'll merge |
Fixes #16
Needs #184 ✔️
Renders #180 obsolete (sorry)
I've finally taken the time to redo my PR for allowing monorepo settings.
I'm introducing a single setting (
SUBPROJECT_ID
) that does multiple things:Of course, it will make a lot more sense after we merge #184 and add
COVERAGE_PATH
(though I believe there may be cases where we may want the 2 settings to be independant: it's not unimaginable to have the coverage run on the root for both projects. It's also reasonable to use COVERAGE_PATH outside of a monorepo setting (e.g. if usingsrc/
) so while the doc forSUBPROJECT_ID
mentionsCOVERAGE_PATH
, I don't believe the 2 settings will be linked in the code.Full backwards compatibility to be expected.