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
252 stars 13 forks source link

Add Rule.withModuleDocumentationVisitor #132

Closed jfmengels closed 2 years ago

jfmengels commented 2 years ago

This PR adds a new visitor to visit the module documentation.

So far, this was done using the comments visitor, which was annoying and not necessarily that approachable to those who don't know how to tell the module documentation from regular comments.

The PR updates some of the rules that I previously worked on that that would benefit from this visitor.

I expect that with the next major version of elm-syntax, the module documentation will be kept separate from the comments, and that the comments visitor will NOT contain the module documentation.

lue-bird commented 2 years ago

Just checking: Does it account for port comments which also start with {-| but are parsed along the other comments? (just taking the first comment starting with {-| could return a port documentation comment if the module doesn't have a documentation comment) Plus, do port documentation comments deserve their own visitors, too, until the next breaking elm-syntax version?

jfmengels commented 2 years ago

Hmm, it doesn't as much as we could.

As far as I understand, the first comment starting with {-| that appears before the first declaration/import is considered to be the module documentation. That means that if you have a module like

port module A exposing (..)

{-| This is a surprisingly a module documentation
-}
port a : String -> Cmd Msg
port module A exposing (..)

import A

{-| This is the port documentation
-}
port a : String -> Cmd Msg

I think this visitor could definitely do a better job at figuring out whether it's module documentation or a port documentation. I'll work on that. Thank you :pray:

jfmengels commented 2 years ago

Plus, do port documentation comments deserve their own visitors, too, until the next breaking elm-syntax version?

I don't think so, because you will rarely want to visit the port and its documentation separately, you will most likely want to visit them together. Having 2 visitors will make it complicated, probably nearly as complicated as it is today.

I'd rather wait to get this for free when the next major version of elm-syntax comes out.

jfmengels commented 2 years ago

@lue-bird I've changed the implementation so that port documentation should not be confused as module documentation. Meaning that using the new visitor would actually fix some potential (though never reported yet) bugs in some of the rules that needed the module documentation.