Conversation
|
So at a high level this seems like a useful feature. I have a hard time thinking about new diff-cover/diff-quality features since I dont really use these tools anymore. I just maintain the project. But it looks like Github annotations introduce some level of customizability (whats a warning, whats an error, whats a message). My gut is the approach taken here "Everything is a warning" seems reasonable. However, if others start using it I suspect the desire for customization will come up shortly after. I think im happy to merge in a "everything is a warning" pr. Do yall think there will be much demand for further customization? Do you think we could get away with "look, the flag will do the basics but if you have more complex demands that config needs to be defined in some config file"? |
|
For the use case like sqlfluff, both errors and warnings would be ideal. Being able to add code suggestions is also useful, though I suspect most people will use a tool like reviewdog instead for that. |
|
Guess who had this same idea today.... [independently] ME! I'll help out, I would like to see this feature soon. |
|
Thanks @kingbuzzman |
|
@timkrins can you sync with main? Is this something you want to finish? Or would you like it/wouldn't mind if i took over? |
|
@kingbuzzman awesome regarding the format arguments. I'm a bit slammed atm but I will try and extract some time to rebase! |
|
No rebase needed, just in github click on the "resolve conflicts" button 😉 |
timkrins
force-pushed
the
github-warning-annotations
branch
from
46661af to
6e573d4
Compare
timkrins
force-pushed
the
github-warning-annotations
branch
from
a5d827c to
a839043
Compare
|
I have rebased on main, and added the annotation format - although right now it is a TODO as I am still a bit unclear on an elegant way to pass the value down to the formatter and then template context. |
|
@kingbuzzman I'll get to it this week :) |
|
@timkrins any idea when you'll get around to this? I kind of need this... If you're unwilling, i'll just merge my PR |
kingbuzzman
left a comment
kingbuzzman
left a comment
There was a problem hiding this comment.
Blocking because of a bugfix inside a feature
.github/workflows/verify.yaml
Outdated
| - name: Complete coverage | ||
| run: | | ||
| poetry run diff-cover coverage.xml --include-untracked | ||
| poetry run diff-cover coverage.xml --include-untracked --format github-annotations:error |
There was a problem hiding this comment.
I'm fine with adding error. But i didn't want to impose it
There was a problem hiding this comment.
@Bachmann1234 what do you think this should be?
There was a problem hiding this comment.
I will change it to use the default, which will be warning.
There was a problem hiding this comment.
There is no "default" it will crash. This is a bigger bug that has little to do with your PR. For now, why dont we use warning and wait for @Bachmann1234
There was a problem hiding this comment.
Ah - I see. I thought it would be a good test of the default, and in a way it was haha.
diff_cover/diff_cover_tool.py
Outdated
| markdown_report = report_formats["markdown"] or MARKDOWN_REPORT_DEFAULT_PATH | ||
| reporter = MarkdownReportGenerator(coverage, diff) | ||
| with open(markdown_report, "wb") as output_file: | ||
| with open_file(markdown_report, "wb") as output_file: |
There was a problem hiding this comment.
Good find, but should really be a separate PR; this should have a test associated with it.
| with open_file(markdown_report, "wb") as output_file: | |
| with open(markdown_report, "wb") as output_file: |
There was a problem hiding this comment.
Ah - I merged in your branch - it was your change.
|
@Bachmann1234 this is Bellissimo!!! and should be [squashed] merged as soon as possible 😇 @timkrins thank you so much for your efforts and idea! This is going to be great for me, you have no idea how much code I will delete because of this! ❤️ |
|
Alrighty! Lets go ahead and merge/release this. Thank you for the PR @timkrins and thanks for the review support @kingbuzzman |
|
Released! Check out https://pypi.org/project/diff-cover/9.6.0/ |
4 participants
Adds a report generator that prints GitHub style annotations to stdout.
Defaults to
warningbut other types (error, info, debug) can be passed (thanks @kingbuzzman for this, and sorry for delay)diff-cover coverage.lcov --format github-annotations:warningrunning in GitHub actions