elm-analyse icon indicating copy to clipboard operation
elm-analyse copied to clipboard

Allow exceptions to ImportAll

Open torhovland opened this issue 6 years ago • 11 comments

It would be nice to be able to configure a few exceptions to ImportAll, i.e. cases where ImportAll is OK. I would prefer to configure Model as an exception. See guideline 2 here for more: https://becoming-functional.com/nine-guidelines-for-modular-elm-development-fe18d2f7885e

torhovland avatar Oct 14 '17 10:10 torhovland

I see your point, but in my honest opinion, I think the author of the article has another alternative. Besides the two options he presents (import Models and import Models exposing (..)) there is a third option: import Models exposing (User, BlogPost).

The problem with the import all syntax (..) is that it does not communicate your code well to other developers that have to maintain the same codebase. With multiple usages of (..) it is hard to figure out where types and functions come from. Importing User explicitly or referring to it via a qualifier (Models.User) will instruct the reader what is happening. In addition readers of your code do not always read in in an editor (think about reviewing pull requests in GitHub), thus this explicitness does matter more for others than for yourself.

In addition, I think having a Models file may indicate smelly code. A different setup of your application's data may be a good choice. See this video.

stil4m avatar Oct 31 '17 20:10 stil4m

I mostly agree with you. I have seen that video, and I also get the idea of postponing the splitting up of the files until you see how the code evolves. I put all my code back into one file, then I identified one aspect of the code that deserved its own module, and extracted that. However, as I'm using Material Design with debois/elm-mdl, I have a lot of view code that depends heavily on the Material module and submodules. Not only that, but several of my view functions are similar in structure. It doesn't feel right to have some of this view code in Main.elm and some in that module I extracted. I also don't like to have all of it in Main.elm. I think the view code deserves a file on its own. But then that file needs to depend on all my Model. I think it makes more sense to factor out the Model then, and let both Main.elm and View.elm depend on that. And this is where I agree with that article that doing exposing (..) should be OK on Model and Msg elements, because otherwise I'll need to basically duplicate most of that like this in Model.elm:

    exposing
        ( AuthenticatedData
        , Model
        , Msg
            ( Authenticated
            , ChangeActivityImage
            , ChangeActivityName
            , ChangeActivitySlider
            , ...

On top of that, I need to have similar imports in Main.elm and View.elm.

So even though in general I like the idea of being explicit, I think it should be OK to do exposing (..) on selected elements.

torhovland avatar Nov 09 '17 06:11 torhovland

Your view modules need not depend on your Model type (or your Message type, either). https://medium.com/@matthew.buscemi/high-level-dependency-strategies-in-elm-1135ec877d49

mbuscemi avatar Nov 09 '17 11:11 mbuscemi

Thank you, that is an interesting approach, quite in line with this idea about dependency rejection: http://blog.ploeh.dk/2017/02/02/dependency-rejection/

However, this requires me to redefine many of the messages in an alias in View.elm. elm-analyse is really very strongly opinionated if it requires me to do this. One of the philosophies of Elm is that it is supposed to be easy and straightforward to use, but this is starting to add complexities that arguably are not needed. On top of that, it seems I cannot even define all those messages I need in that alias. When using elm-mdl, one of the messages is Mdl (Material.Msg Msg). So in View.elm the compiler tells me I will need:

type alias Messages message =
    { materialMsgHandler : Material.Component.Msg -> message
    }

However, Material.Component is not exposed, as can be seen here: https://github.com/debois/elm-mdl/issues/272

So, all in all, I still think the approach from https://becoming-functional.com/nine-guidelines-for-modular-elm-development-fe18d2f7885e is the best one in my case, and for that I still think elm-analyse should support exceptions to ImportAll.

torhovland avatar Nov 09 '17 19:11 torhovland

I got this working in the end. The compilation issue above was resolved like this:

type alias Messages message =
    { materialMsgHandler : Material.Msg message -> message
    }

However, after I got the code refactored, I didn't like having this type alias mirroring about half of the messages in my main Msg, so I refactored it away and put individual message handlers as function arguments instead. For a few of my view methods this means rather long-winded type annotations, such as this:

editActivityPageView :
    (Material.Msg msg -> msg)
    -> (String -> msg)
    -> (String -> msg)
    -> (Float -> msg)
    -> msg
    -> Material.Model
    -> ActivityEdit
    -> Html msg
editActivityPageView materialMsg changeName changeImage changeSlider saveActivityType mdl activityEdit =

But for others, it is clean and simple. And calling such a function from Main.elm is as simple as:

            View.editActivityPageView
                Mdl
                ChangeActivityName
                ChangeActivityImage
                ChangeActivitySlider
                SaveActivityType
                model.mdl
                model.activityEdit

And my code is nicely separated now, without excessive import/export exposing lists.

I'm happy with this solution. Thank you for the suggested improvements. I still think that this kind of code separation may not appeal to everybody, and that my original suggestion would be useful in those cases, but you're of course entitled to disagree with that.

torhovland avatar Nov 12 '17 07:11 torhovland

I'd like to add another voice for, at least, being able to use import all on union types (see an example of this usage in this project's app). I agree, in general, with the clarity that specifying imports gives, but there are cases where it doesn't seem like it ends up adding value

bdukes avatar Nov 16 '17 15:11 bdukes

Personally, I have found the strictness of this check to be beneficial to my Elm projects. There have been instances where I have questioned the rules validity, but in every case it has forced me to learn better ways of structuring my code so that aren't so that there aren't a very large number of imports and exports in a single module.

mbuscemi avatar Nov 17 '17 00:11 mbuscemi

I've made this check less strict in the new version of elm-analyse (0.13.0).

import Foo exposing (..) // Still a violation
import Foo exposing (Bar(..)) // Not a violation

stil4m avatar Dec 06 '17 18:12 stil4m

I find myself wishing I could add exceptions (a la TriggerWords) to this, specifically for Html or elm-ui's Element. Even in the introduction to Elm guide, Evan mentions that he imports all from Html:

I tend to use it for import Html exposing (..) but not on anything else.

This is reasonable to me -- these libraries read as DSLs, or replacements for HTML. Specifically, these two read quite differently to me:

div []
    [ img [ Attributes.src "hello.png" ] []
    , div [] [ text "hello" ]
    ]

Html.div []
    [ Html.img [ Attributes.src "hello.png" ] []
    , Html.div [] [ Html.text "hello" ]
    ]

And doing a list of imports gets out of hand quite quickly. As an example, one of my modules has this import from Element right now.

import Element exposing (Element, alignLeft, alignRight, alpha, centerX, centerY, column, el, fill, height, html, htmlAttribute, layout, mouseOver, padding, paddingEach, paddingXY, px, row, spacing, text, toRgb, width)

Curious if other people feel the same way about this!

bengolds avatar May 17 '19 18:05 bengolds

I tend to use it for import Html exposing (..) but not on anything else.

Curious if other people feel the same way about this!

Yes, I came here to suggest the exact same thing. The only module I ever want to use exposing (..) with is Html and I will forever have linting errors in most of my modules w/ views until I can whitelist Html :/

prikhi avatar Sep 17 '19 17:09 prikhi

I agree with @bengolds and @prikhi. The Html import violations clutter my elm-analyse messages a lot.

It would be cool if either

  • it would be possible to configure exceptions, or if
  • exposing (..) from Html, Html.Attributes and Html.Events would by default not present a violation.

davidpomerenke avatar Jun 22 '20 14:06 davidpomerenke