Add annotations via console - #20
Conversation
ggilder
left a comment
There was a problem hiding this comment.
Sorry for the delay in reviewing this, I wasn't getting notifications for some reason!
This looks awesome and simplifies the code a lot — thanks for submitting! I left a small comment about additional cleanup we can do for the annotate method now, and I think the PR needs to be rebased/distribution files regenerated since I merged another change a few days ago.
I do have just one concern — I ran into issues previously with PRs with large numbers of annotations (> 50) which necessitated the batched API calls. Would love to see a demonstration of > 50 annotations on a PR using this console method just to make sure that is still supported.
Thanks!
Uh oh!
There was an error while loading. Please reload this page.
It's documented that the limit of 50 annotations is for the batch API |
7b3f3e0 to
ae50976Compareggilder
commented
Dec 24, 2024
Changes look good! Let me know if you're able to link to a demo PR showing > 50 annotations |
@ggilder Hi! I crated a two similar PR and example repo as a demo.
|
numbata
commented
May 20, 2025
While annotations are helpful, flooding a pull request with thousands of "uncovered line" notes hurts readability. Console publication does have limits, but here that’s a plus: it keeps reviews focused and lets the coverage workflow run with a leaner permission set. Alternative: If some contributors still want the full list, we could add a simple switch - |
ggilder
commented
May 29, 2025
@numbata hey, thanks for chiming in and providing those examples, that helps clarify the tradeoffs here. I have to disagree with the idea that limiting annotations to 10 is a good default behavior. That's super low, especially when you consider that you could easily have a single function use up that entire quota depending on the language and how line breaks/conditionals are used. I wouldn't want the user to get a false sense of security, e.g. perhaps they're fine with that single function being uncovered so they move on assuming that the rest of the PR is good, when really they should be alerted to other uncovered code. The whole point of this tool is to reduce cognitive load for pull request reviewers, and having to think about "did I already see 10 annotations?" is not, in my opinion, a good use of a pull request reviewer's mental bandwidth. That said, I'd still be happy to include this change strictly as an opt-in mechanism, with these tradeoffs very clearly documented, for those who want it. If you're still interested, I encourage you to add a switch that defaults to the current behavior and allows users to opt-in to the console behavior. This pull request is also out of date with the main branch so those conflicts would need to be addressed. Again, thanks for your effort on this, and I hope this is a workable path forward for you. |
numbata
commented
May 29, 2025
I’m totally fine with offering an annotation mode strictly as an opt-in feature rather than changing the default. In my own reviews, seeing a handful of uncovered spots is enough of a signal to dig deeper—any more line-by-line comments start to clutter the PR and distract from the bigger picture. |
I only recently started to use this action (since codecov stopped working for me), and haven't tested forks yet. |
ggilder
commented
Sep 10, 2025
@HarelM I haven't tested with forks, but generally to use the annotations API you need permission to write "check" information. Console output can be used in this fashion as well but has limitations, e.g. can't output more than 10 annotations. |
That is correct and that was the motivation for this change, as the console doesn't need any API permissions. I do agree that giving the option to switch mode or maybe having it as a fallback if there is no write permission would be better. Like this your own PR's would have the 50 annotations but the external PR's would only have the 10 that the console creates |
HarelM
commented
Sep 14, 2025
While I still need to figure out if I want to add the relevant permissions or not, here an interesting use case from my point of view where I PR someone contributed had 1 annotation needed (i.e. only one place where coverage was missing) but from the console it's not clear where this place is: So I think this feature is needed, and I would also say it should be on by default. If there are too many missing coverage it can say that the console report was deducted due to too many missing places or something and present the first X to avoid printing too much to the console. |
ggilder
commented
Sep 15, 2025
@HarelM yeah that could be a way forward here — if the action is unable to access the annotation API, it could fall back to console and issue a warning if there are more annotations than the console limit. |
HarelM
commented
Sep 15, 2025
I think that would be a great middle ground. |
numbata
commented
Dec 22, 2025
Is there any chance to have this as a part of some |
ggilder
commented
Dec 23, 2025
@numbata absolutely, the PR needs some work though. Namely the switch to have console output be opt-in/fallback as discussed earlier in this thread. |
This allows the annotations to be written without an API call.
Motivation: the API call requires
checks: writepermission to the repository and so doesn't work in a PR of a forked repoHere it is the console log in action
https://github.com/ismail9k/vue3-carousel/actions/runs/12272312279/job/34240844912?pr=450