search

crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.22k stars 1.61k forks source link

Enable `ameba` in this repo #14608

Open straight-shoota opened 1 month ago

straight-shoota commented 1 month ago

https://github.com/crystal-ameba/ameba is a great code analysis tool for Crystal. It has some limitations because it only operates on the syntax level. We've had some improvements based on ameba's suggestions in the past (#12685, #12730, #12637, #12648 and more).

It should be easy to set up ameba to run automatically on this repo for continuous monitoring of new code submissions. I think there's a huge gain from ensuring we avoid a wide range of simple errors that ameba can detect.

The code base is pretty huge and there are a lot of violations of ameba rules. But that shouldn't be a problem. Ameba can automatically analyize the current state and add exclusions for existing linter issues. That would let us start using ameba on new code additions already before fixing everything in the existing code.

We could also consider disabling some default rules entirely and re-evaluate in the future. I see two reasons for this:

So I'd suggest the following plan:

  1. Add a basic .ameba.yml config with exclusions for all current problems (via ameba --gen-config).
  2. Discuss which rules we may want to disable entirely (permanently or temporarily). (or maybe the other way around: start with a blank slate and discuss which rules we want to enable?)
  3. Add an ameba job to the CI config to ensure we catch problems in code submissions
  4. Start fixing the reported problems, one rule at a time. Starting with the low-hanging fruits which ameba can autofix (ameba --fix).

Any other ideas? /cc @Sija @veelenga

devnote-dev commented 1 month ago

Please in any case, disable Metrics/CyclomaticComplexity. šŸ™

Blacksmoke16 commented 1 month ago

Related to https://github.com/crystal-ameba/ameba/issues/448, we should def take a pass on enabling only the most useful ones to start; disabling the newer, more subjective ones.

HertzDevil commented 1 month ago

We've had some improvements based on ameba's suggestions in the past

we avoid a wide range of simple errors that ameba can detect

My first impression of Ameba is that it is indirectly responsible for Crystal's 1.7.1 patch release, so while enforcing some kind of consistent coding style may be good on its own, I don't agree with the notion of using a syntactic linter to catch "errors".

straight-shoota commented 1 month ago

@HertzDevil Yeah, error might not be the best word. Let's say ameba can help us consistently enforce some good practices?

ysbaddaden commented 1 month ago

I agree that Ameba is useful. My only concern is that there are no prebuilt executables, and it takes a couple minutes to recompile it on every CI run.

Note: Lint/NotNil is annoying... until you realize you can pass an explanatory message as #not_nil!(message) and then it stops complaining and it's much better. That doesn't mean we can enable it (yet), but it took me a while to notice that.

veelenga commented 1 month ago

@ysbaddaden I think we can prebuilt the executables and host it somewhere. Would Github packages work?

straight-shoota commented 1 month ago

Caching builds shouldn't be an issue. We could easily do that ourselves with actions/cache. Or use pre-built binaries from nixpkgs, for example.

Wouldn't hurt to have this handled upstream, though @veelenga. There's even https://github.com/crystal-ameba/github-action for GitHub actions, but IIUC it needs to rebuild the container every time? At least reusage doesn't seem very good, right?

ysbaddaden commented 1 month ago

I knew I reported that already :grin: https://github.com/crystal-ameba/github-action/issues/18

@veelenga or github releases or anywhere. having executables ready to be installed on CI or docker images or our local computer would be :heart:

Sija commented 1 month ago

I think that having ameba enabled is a great idea. I can make a pass at it, as I've already started the process some time ago ;)

straight-shoota commented 1 month ago

@ysbaddaden https://github.com/crystal-lang/crystal/pull/14631#issuecomment-2134630661

It needs the token to report Ameba issues as comments on the PR.

I didn't know that's a thing. Is this the default behaviour of the ameba action? Why would we want that, though? šŸ¤” Would the check result from the GHA workflow not be sufficient?

ysbaddaden commented 1 month ago

The action acts as a PR reviewer. It's nice to have the messages right into the code (that can/will be resolved) rather than having to reach for the CI logs. I guess it could also propose fixes as suggestions (I'm not sure it does that).

straight-shoota commented 1 month ago

It's nice to have the messages right into the code (that can/will be resolved) rather than having to reach for the CI logs.

This should work directly with GitHub actions via annotations or something. There should be no need for the CI workflow to post PR review comments.

IIRC, this is already working for compiler warnings, for example.

ysbaddaden commented 1 month ago

Oh maybe they're annotations? anyway, it still needs a github token to do that :)

straight-shoota commented 1 month ago

I'm pretty sure you don't need a token for that. You can create annotations manually by writing some specific markup to stdout: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-a-warning-message

I do not recall how exactly this works for compiler warnings, but I don't think we do anything explicitly like that somewhere.

veelenga commented 1 month ago

@straight-shoota the action uses a GitHub API to create a check-run and annotations. And token is required to do that.

https://github.com/crystal-ameba/github-action/blob/master/src/ameba-github_action/runner.cr#L3

This mechanism was developed before the capability to create annotations using stdout messages was introduced.

straight-shoota commented 1 month ago

Anyway, I think getting direct feedback on code locations is an advanced feature. We don't need that in an initial implementation.

As mentioned in the OP, I think the first step should be just adding a .ameba.yml configuration. Then a simple CI job which just fails and prints the violations should be good enough for a start. And of course you can use ameba locally as well.

veelenga commented 1 month ago

@straight-shoota @ysbaddaden could you please let me understand if there are still blockers to moving this forward on Ameba's side? From what I understand we would like to:

  1. Have precompiled binaries
  2. Use GitHub action with a precompiled binaries + avoid using GITHUB_TOKEN

Are those blockers, or would you like to move with #14631, and the two above can be resolved later?

ysbaddaden commented 1 month ago

They'd be great to have and improve on the situation for everyone, but I believe they're not blockers.

straight-shoota commented 1 month ago

Yeah, definitely not blockers.

However, I would prefer not to install ameba as a development dependency with shards. It should be built independently as a development tool, and we can cache the binary within GitHub actions.

We might also look if binary artifacts are available anywhere else. I suppose homebrew isn't cached because it's only available as a tap. Nixpkgs should work, though šŸ¤”