NoModuleOnExposedName fix can introduce name collision about elm-review-imports HOT 6 OPEN

Ooo that's an interesting case there. I don't think this rule should be responsible for fixing your exposing (that's for another rule in this package perhaps) but I'll certainly take a look to remove the auto-fix in this case.

from elm-review-imports.

To be clear, I think the second example shouldn't be an error at all due to there being no good way to fix it. I agree the first example should just not have an auto-fix though.

from elm-review-imports.

There are a number of 'good' ways the second can be fixed... the author could choose not to expose the Subcomponent's Msg variants and fully qualify all their subcomponent use or they could choose to rename the local Msg type so there's no shadowing.

What happens if the author decides to add MsgVariantInSubComponent to a local type or import it again from another module? The compiler will then complain about a type mismatch, the author will need to prefix the module name, and the exposing becomes further pointless.

Personally, I'm not a fan of imports exposing type constructors because it makes it much harder (even than normal imports) to figure out where things are coming from.

from elm-review-imports.

I agree with @sparksp that there are multiple ways to fix the issue, qualified imports of the constructors being one of them.

But I think that the original example as it is is just fine too and that there is not a lot of ways the situation can be improved upon without potentially a lot of (manual) renaming. I therefore think the rule should not report an error in the case where the imported type is the same as a local type (and probably the same thing for values?).

from elm-review-imports.

@sparksp Fair enough, I guess I'm okay with it showing an error but not an auto-fix in the second example as well. That said, if it is an error and you're feeling particularly motivated, maybe NoModuleOnExposedName could detect this situation and suggest to the user to not have Msg(..) and instead provide qualified names to the Msg variants.

from elm-review-imports.

Ran into this situation as well, today.

Seems like folks have settled on "Don't report an error" in this case. Has anyone made any progress on adding that behavior to the rule?

from elm-review-imports.