Add configuration for only posting warnings to added/modified lines#89
Add configuration for only posting warnings to added/modified lines#89Phillita wants to merge 31 commits into
Conversation
Co-authored-by: Bert McCutchen <bert.mccutchen@gmail.com>
|
@Phillita can you rebase your PR on the latest |
|
@Phillita I'm not sure the merge was correct, because it hasn't run the checks -- I normally get an option to approve them, but on this PR even that option is missing. |
Strange! It says that the workflow requires approval from a maintainer. I'm not quite sure what that means but I'll try looking into it when I get to my laptop. |
@ashkulz Does it allow you to trigger the workflow run? I'm seeing this in the Github documentation but I have limited knowledge on this:
|
|
It doesn't, this is what I see:
|
|
Ah, there was an extra button in the "GitHub Actions" run and now it seems to be running:
|
ashkulz
left a comment
ashkulz
left a comment
There was a problem hiding this comment.
I'd appreciate if you could fix the failing tests before I did a thorough review on the PR, since that could cause further changes.
Rubocop has been defined in the gemfile for development and test use.
I pushed a fix for the duplicate gem. I missed this being added during the merge. Locally the tests run. |
|
I understand what's going on now. The workflow adds specific versions of rubocop to the gemfile. This conflicts with me moving the gems out of the gemspec for local dev. I'm going to try pushing up a change to the workflow to control this through bundler. I'm not sure if this will work but it's worth a shot. |
|
@ashkulz nevermind, I reverted my gemfile change for rubocop. Let's see if the actions are happy with this. |
|
Base64 has been added back into the gemfile. 1 last run will hopefully do the trick |
| offense.message.gsub( | ||
| offense.cop_name, "[#{offense.cop_name}](#{documentation_url})" | ||
| ) |
There was a problem hiding this comment.
concern: does this affect CLI output as well? I'd want to differentiate between what is shown on the CLI vs what's posted as Markdown.
There was a problem hiding this comment.
This will affect the CLI. Do you have a suggestion for how we can differentiate the output format based on where pronto is being run?
Here is an example run on some staged test code locally:
I don't think this is a bad thing, though. It provides an easy way to get to the documentation from my terminal.
There was a problem hiding this comment.
In the CLI, I'm more interested in seeing compact output so that it doesn't go over 80 (or whatever terminal width people have configured). Maybe we can keep message_text as-is (after removing suggestion_text) and use it for the CLI and introduce message_markdown (which includes suggestion_text and these changes) and use it in the appropriate formatters?
I know that's a large refactoring, but I think it'd be valuable in this case -- since most of the time suggestions are not enabled in the CLI but only in CI, this change would have an impact on people who use pronto via pre-commit hooks.
There was a problem hiding this comment.
I'll have to look into how pronto passes formatters to the runners. I'm unsure about this at the moment.
I agree it would be much more compact. I have a lot of experience with Github but I lack knowledge with bitbucket/gitlab. I don't know which ones would use markdown.
It makes me wonder, would it be a good idea to add a config flag that turns markdown on and let the end user to control this?
There was a problem hiding this comment.
I think all of them use Markdown, if you do it for GitHub I think it'll be obvious how to do it for the others as well. I don't think we need a configuration flag as such, it should depend on the formatter.
There was a problem hiding this comment.
@ashkulz I think I need a little more direction here. (Sorry in advance if I'm being dense)
When pronto runs, each runner is called without formatter context.
Looking at how the github formatter works, it uses the messages from the runners without any additional parsing.
It creates Comment objects here.
I'm confused about how to use the chosen formatter to get the proper message from the runner (with/without markdown).
Do you have context for how I could do this?
Bump the minimum version of rubocop for the gem to the earliest date for the documentation_url method.
Updated specs. Removed conditional code based on min rubocop version update.
|
@ashkulz Sorry for the delay. I've removed checks for anything below rubocop 0.90.0 since we've updated the base version for the gem. |
|
@Phillita sorry, but due to a family medical emergency I'm not going to be able to review this immediately. I'll try by this weekend 🤞 |
No problem at all. I hope everything is ok. |
There was a problem hiding this comment.
Did a quick review, will also reply to your other comment later on in the week.
|
@ashkulz I've applied your suggestions. Review whenever you are able. No rush! |
Remove ruby 2.3 from the matrix. Include ruby 2.4/2.5 checks instead of using the exclude rules.
|
@ashkulz It's been a while! I hope everything is all good on your end. Please let me know if you'd like to continue discussion. |
|
@Phillita sorry for the delay, things have been crazy. I'll get back to this over the weekend. |
|
@ashkulz sorry but my workplace has switched away from pronto. This is no longer actively in use. |
4 participants
First off, thank you for your work on Pronto/Pronto Rubocop!
Problem:
I'm looking to add functionality to this gem that hides the external Rubocop warnings for unchanged lines of code.
This is to help control the intake of warnings that are generated by new code without having to resolve warnings around it.
Example:
The file in question has a "Class has too many lines" Rubocop warning.
Updating a method within this class that didn't cause this warning directly, is now receiving a Github Review comment with this warning.
Solution:
enabled: true || falserange: nwhere n is added to the warning start to broaden how many warnings are caughtCaveats:
If new code generates a warning that starts outside of the patched lines, and no range has been provided, it will be ignored.
The range option has been added to capture some of these but not all will be caught.
Getting a delta of warnings added before the new code would be the best outcome here, but that seemed non-trivial.
UPDATE - April 18, 2024
We've been running into issues with confusing offences showing up on lines that they are not directly connected to. To handle this, I've added in some messaging to show the line that the message/suggestion applies to. I've also made it so that suggestions will only be given to lines where the offence directly maps to the code line.
UPDATE - June 18, 2024
I've added in documentation links from the github pull request.