Enable ameba in this repo

[straight-shoota]

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:

• Disable frequently violated rules for efficiency. For example, Lint/NotNil, Naming/BlockParameterName, Naming/VariableNames each report over 400 problems. It's going to be hard to fix them all in the short run.
• Some of the default rules may be a bit too opinionated.

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]

Please in any case, disable Metrics/CyclomaticComplexity. 🙏

[Blacksmoke16]

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

[HertzDevil]

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]

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

[ysbaddaden]

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]

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

[straight-shoota]

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]

I knew I reported that already 😁
crystal-ameba/github-action#18

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

[Sija]

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]

@ysbaddaden #14631 (comment)

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]

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]

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]

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

[straight-shoota]

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]

@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]

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]

@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]

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

[straight-shoota]

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 🤔