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

Namespace maintenance #98

Closed
pravdomil opened this issue Apr 6, 2021 · 5 comments
Closed

Namespace maintenance #98

pravdomil opened this issue Apr 6, 2021 · 5 comments

Comments

@pravdomil
Copy link

pravdomil commented Apr 6, 2021

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
Copy link

lydell commented Apr 7, 2021

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

@jfmengels
Copy link
Owner

Hi @pravdomil 👋

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
Copy link
Author

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
Copy link
Author

pravdomil commented Apr 8, 2021

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
Copy link
Author

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

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

3 participants