Enable `ameba` in this repo about crystal

[devnote-dev]

Please in any case, disable Metrics/CyclomaticComplexity. 🙏

[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.

[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.

[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 ❤️

[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?

[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 ;)

[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).

[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.

[ysbaddaden]

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

[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?

[veelenga]

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

[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?

[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.

[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?

[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 🤔