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

Make it easier to see linter issues #368

Merged
merged 3 commits into from
Mar 14, 2024
Merged

Conversation

MichaelSnowden
Copy link
Contributor

What changed?
I changed the format of the api-linter to:

  • Only show the rule URL and location
  • Make the location a clickable link
  • Group violations by rule

Why?
The output previously was not very easy to parse:

Before:

Screenshot 2024-03-13 at 12 55 27 PM Screenshot 2024-03-13 at 12 55 39 PM

After:

Screenshot 2024-03-13 at 12 54 45 PM

Breaking changes

Makefile Outdated
@@ -104,7 +94,7 @@ buf-install:
##### Linters #####
api-linter:
printf $(COLOR) "Run api-linter..."
$(call silent_exec, api-linter --set-exit-status $(PROTO_IMPORTS) --config $(PROTO_ROOT)/api-linter.yaml $(PROTO_FILES))
@api-linter --set-exit-status $(PROTO_IMPORTS) --config $(PROTO_ROOT)/api-linter.yaml --output-format json $(PROTO_FILES) | jq -r 'map(select(.problems != []) | . as $$file | .problems[] | {rule: .rule_doc_uri, location: "\($$file.file_path):\(.location.start_position.line_number)"}) | group_by(.rule) | .[] | .[0].rule + ":\n" + (map("\t" + .location) | join("\n"))'
Copy link
Contributor

Choose a reason for hiding this comment

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

The jq requirement might be problematic. I don't know if we can expect everyone to have that installed?

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, since we expect go to be installed, there seems to be a Go port (https://github.com/itchyny/gojq) which could be installed like the other tools.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead of requiring jq, you can add a separate target api-linter-pretty and recommend running that for pretty output if this target fail.

@cretz
Copy link
Member

cretz commented Mar 13, 2024

Wish we didn't have to have platform dependent build scripting and require extra dependencies like "jq". We have a perfectly fine language for executing processes, parsing JSON, etc.

@MichaelSnowden
Copy link
Contributor Author

Thanks for the suggestion, @stephanos! gojq seems to virtually be a drop-in replacement for jq. Also, the install already has a good home in api-linter-install. Hopefully this addresses your concerns about portability, @cretz and your concern about installing jq @bergundy.

@cretz
Copy link
Member

cretz commented Mar 14, 2024

👍 Still unfortunate that we have platform-specific build scripts, but that's not specific to this issue

@MichaelSnowden MichaelSnowden merged commit 26d5618 into master Mar 14, 2024
3 checks passed
@MichaelSnowden MichaelSnowden deleted the snowden/make-linter branch March 14, 2024 22:52
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.

None yet

4 participants