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

ameba --gen-config produces a mess #566

Open
straight-shoota opened this issue Feb 15, 2025 · 2 comments
Open

ameba --gen-config produces a mess #566

straight-shoota opened this issue Feb 15, 2025 · 2 comments

Comments

@straight-shoota
Copy link
Contributor

straight-shoota commented Feb 15, 2025

When adding ameba to a project, it seems like a good idea to run ameba --gen-config to get a neat starting point.
The idea is that this command generates a .ameba.yml configuration that records all rule violations that currently in the code base. That gives you time spread the adoption process and fix existing issues over time while ideally avoiding to add additional ones.

However I've found this process is not very straightforward and the autogenerated configuration has some major issues.
This means a great deal for user experience, especially when trying to adopt ameba into an existing codebase of non-trivial size.

As a practical example, I have run ameba --gen-config on the shards (using ameba 1.6.4). The codebase is about 8K sloc big and ameba found 383 violations. It produces a configuration file that's 357 lines long.
https://github.com/straight-shoota/shards/blob/011ddeef104e996d01ffd774f437ae0467688c7e/.ameba.yml

  1. The entries in the config file are in complete disorder. It seems they are sorted by filename of the violations or something like that. When removing the violation of Documentation/DocumentationAdmonition in the first reported file (src/helpers.cr), a newly generated config removes that exclusion, but now the entries are in a different order. That's very inconvenient for making iterative changes.
    Configuration entries should be ordered by rule name. That would also group rules based on category (lint, naming, style etc.).

  2. The level of detail in the generated default configuration is surprising.
    What am I to make of a list of 17 AllowedNames for the Naming/BlockParameterName rule? Are those representing the default configuration? If so, why would I want it to be printed out? I would usually trust the default configuration and don't want to mess with it unless I have some specific need for it.
    Also every config entry has a Severity property. It's nice that I could change the severity of individual rules, but I figure there would rarely be a need to. Nor do I see a need to have this available for informational purposes. Most users will be happy with whatever the default is. No need for this noise.
    Another issue is that writing out the current default configuration values freezes them. Should the default change for some reason in future versions, this config setting would still explicitly reference the old default. This might have some value in terms of consistency. But written out default value is distinguishable from an explicit override. Without proper change management you might miss useful updates in future versions.
    The current develpment version of ameba adds another curious configuration property for all entries: SinceVersion. This could be useful information, I guess. Not sure what exactly it would help with, though. But the more pressing question: Why is this a configuration option that I could presumable change? What would happen if I changed this setting?

  3. The configuration entries seem to mix up different concerns which I think should better be handled separately.

    1. Every config entry has some settings that allow me to customize how ameba should treat the respective rule. I can disable it entirely or change some of the options. This is entirely information what you would expect in such a configuration file.
    2. Ancillary data like a preceding comment that reports the number of violations: # Problems found: 20. Perhaps that might be useful to know, but it's completely transient. It describes the state at the time this configuration was generated (so it might make sense to print to stdout at that time). The situation might be entirely different every time someone reads this comment. There's not really any value in persisting this information.
    3. Somewhere in between is temporarily stable data: The exclusion paths for currently existing violations. These I'd like to commit to the repository, but they're intended to be resolved eventually. But this conflicts with completely static configuration of exclusion paths. Maybe I never want some rule to run in a specific path because it contains code that intentionally violates the rule and this will never change. The same applies to completely disabling rules. Of course I can use comment to tell these use cases apart and maybe explain the reasoning. But that's not easy for machines to understand. And it feels that a description of a transitional state should better be separated from static configuration. Rubocop for example generates a separate .rubocop_todo.yml.
@Sija
Copy link
Member

Sija commented Feb 15, 2025

I believe you've raised important DX issue here. I've been thinking more-or-less the same since some time but never actually attend to the matter. I'm all for removing all but the vital parts of the auto-generated config and split it up, potentially.

ad 1: This should be resolved by #528

@straight-shoota
Copy link
Contributor Author

ad 1: This should be resolved by #528

Ehm this still sorts by source file, doesn't it? So it might bring a bit more stability (same source tree leads to same order), but order still depends on the source tree (changes in the source tree can lead to a different order).

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

No branches or pull requests

2 participants