jfmengels / elm-review

Analyzes Elm projects, to help find mistakes before your users find them.
https://package.elm-lang.org/packages/jfmengels/elm-review/latest/
Other
255 stars 13 forks source link

Namespace maintenance #98

Closed pravdomil closed 3 years ago

pravdomil commented 3 years ago

Looking at elm review rules, it would be handy to establish a namespace for rules, something like Review.Rule.Xyz. Than we can easily for search for rules by namespace prefix. And second, rules gets automatically categorised, if we spot "Review.Rule.NoAlways" we know that is a review rule.

image

What do you think?

lydell commented 3 years ago

The screenshot is from https://elm.pravdomil.com/packages/

jfmengels commented 3 years ago

Hi @pravdomil :wave:

I see little benefits in doing this. There is already a convention of naming the packages with a name starting by elm-review-, which I think is an okay way to find rules. It would be nice to improve it, especially since not all packages have adopted this naming convention unfortunately, but I don't think your suggestion is the best way to make them more searchable.

One reason for this is that Review.Rule and more generally Review.XYZ is the namespace for the elm-review API. Having rules adopt Review.Rule.Xyz would be confusing.

Another reason is that rule names tend to already be very long (see this one for instance) compared to other module names.

Then these would all end up in the review/ReviewConfig.elm file, making the module a lot more verbose. So instead of

config : List Rule
config =
    [ NoUnused.CustomTypeConstructors.rule []
    , NoUnused.CustomTypeConstructorArgs.rule
    , NoUnused.Dependencies.rule
    , NoUnused.Exports.rule
    , NoUnused.Modules.rule
    , NoUnused.Parameters.rule
    , NoUnused.Patterns.rule
    , NoUnused.Variables.rule
    ]

you'd have

config : List Rule
config =
    [ Review.Rule.NoUnused.CustomTypeConstructors.rule []
    , Review.Rule.NoUnused.CustomTypeConstructorArgs.rule
    , Review.Rule.NoUnused.Dependencies.rule
    , Review.Rule.NoUnused.Exports.rule
    , Review.Rule.NoUnused.Modules.rule
    , Review.Rule.NoUnused.Parameters.rule
    , Review.Rule.NoUnused.Patterns.rule
    , Review.Rule.NoUnused.Variables.rule
    ]

I think the more consistent naming at the moment for elm-review rules is the fact that rules are named rule. I think that would be more interesting and less annoying for both maintainers and users.

pravdomil commented 3 years ago

That is right.

I was planning one day to have "module search". Instead search by package name, search by module name and list all modules from all packages together (as discussed on slack). And having reserved prefixes: "Review", "Api", "Ui", "String", "Csv", "Iso3166". They will serve as categories.

But with the current state, it will not work, because there is no module name prefix. Maybe I can do special case for elm-review, to add invisible prefix to modules.

This way we don't have to worry about package names and module names and just module names. But that is maybe issue for https://github.com/elm/package.elm-lang.org/.

For example Base64 works really nicely. If I want to create Base64 library I start the namespace with Base64. And will align with other libraries nicely.

pravdomil commented 3 years ago

So in the end I join Review.* namespace with No[A-Z]* and that solves the thing in some way (you can close the issue). You can now see all elm-review related stuff on https://elm.pravdomil.com/packages/ (group by modules and search "review").

Also I realise that relying on module name is easier then on package name, you can change module name with new release, but changing package name is no that easy.

Screenshot from packages browser:

pravdomil commented 3 years ago

Well I got some false positives with NoRedInk namespace. So now NoRedInk modules are considered elm-review modules.