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: marker as argument #180

Closed

Conversation

ArzelaAscoIi
Copy link

@ArzelaAscoIi ArzelaAscoIi commented May 31, 2023

  • added the "marker" argument to enable multiple comments for multiple services in a single repository
  • adjusted tests for a custom marker
image

@github-actions
Copy link

End-to-end public repo
End-to-end private repo
If the tests fail, @ArzelaAscoIi will be added to the private repo

@github-actions
Copy link

github-actions bot commented May 31, 2023

Coverage report

The coverage rate went from 100% to 100% ➡️
The branch rate is 100%.

100% of new lines are covered.

Diff Coverage details (click to unfold)

coverage_comment/template.py

100% of new lines are covered (100% of the complete file).

coverage_comment/settings.py

100% of new lines are covered (100% of the complete file).

@ArzelaAscoIi ArzelaAscoIi marked this pull request as draft May 31, 2023 07:45
@ArzelaAscoIi ArzelaAscoIi changed the title WIP feat: marker as argument feat: marker as argument May 31, 2023
@ArzelaAscoIi ArzelaAscoIi marked this pull request as ready for review May 31, 2023 08:37
Copy link
Member

@kieferro kieferro left a comment

Choose a reason for hiding this comment

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

Thank you for your PR. Looks good to me so far.

An addition to the README would still be nice so that the docs contain the new parameter. I guess this is then also to be used in conjunction with COVERAGE_DATA_BRANCH so that coverage data can be collected for several applications in one repo? This could perhaps also be written specifically in the README.

@kieferro kieferro requested a review from ewjoachim May 31, 2023 22:26
Copy link
Member

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

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

Nice :) Same as the other PR, If you can rework the commits a bit, it would be great but feel free to mention if you'd rather we do it.

@@ -45,6 +43,7 @@ def get_comment_markdown(
previous_coverage_rate: decimal.Decimal | None,
base_template: str,
custom_template: str | None = None,
marker: str = "<!-- This comment was produced by python-coverage-comment-action -->",
Copy link
Member

@ewjoachim ewjoachim Jun 1, 2023

Choose a reason for hiding this comment

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

I guess this might be a mandatory parameter, to avoid hardcoding the default value in multiple places more than necessary

@ArzelaAscoIi
Copy link
Author

Thanks for the review! I will adjust the docs + and rework the commits next week :)

@ewjoachim
Copy link
Member

Interestingly, I just realized that I did a branch 4 months ago that I never finished, which did the same thing. Here's the work I did, might inspire you :)

https://github.com/py-cov-action/python-coverage-comment-action/tree/monorepo

@ewjoachim
Copy link
Member

(Just so you know, I've finally openned the PR #213 based on the branch I mentionned above. It does things a bit differently, using a single setting to take care of all monorepo issues)

@ArzelaAscoIi
Copy link
Author

Ahh for not getting back to it. So let's close this one and follow your approach :)
Thanks!

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.

3 participants