elm-analyse
elm-analyse copied to clipboard
Allow exceptions to ImportAll
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
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.
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.
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
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.
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.
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
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.
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
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!
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
:/
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 (..)
fromHtml
,Html.Attributes
andHtml.Events
would by default not present a violation.