avh4 / elm-format

elm-format formats Elm source code according to a standard set of rules based on the official Elm Style Guide
BSD 3-Clause "New" or "Revised" License
1.31k stars 145 forks source link

Automatically generated 'exposing' block feels like a hindrance #573

Open bburdette opened 5 years ago

bburdette commented 5 years ago

Working in a small project where I'm quickly adding a lot of functions and renaming things often

Lately I noticed that elm-format has started replacing this:

module Staff exposing (..)

with this

module Staff exposing (Accidental(..), AccidentalSymbol(..), KeySig, KsLookup, MidiNotes, PcNote, PitchClass(..), Staff, StaffNote, aToAs, adjacentPitch, aflatmajor, allPitchClasses, allkeys, amajor, bassStaff, bflatmajor, bmajor, cmajor, dflatmajor, dmajor, dropEvry, eflatmajor, emajor, fmajor, gmajor, intToPc, ksLookup, ledgerLine, ledgerLines, makeTrebleStaff, midiToPcNote, midiToStaffNote, note, pcDEmpty, pcIndex, pcLookup, pcNoteToStaffNote, pcToInt, pitchClassEmptySet, staffToY, staffline, toNotes, trebleStaff, view, what)

In my current project I've got several small modules and I'm iterating on my design quickly, and I don't really want to maintain the 'exposing' clause at the moment. I find myself going up there and deleting it several times a day so that elm-format will replace it with the full clause.

You know what would be cool?

If you did this:

module Staff auto

or maybe:

module Staff where

Then elm-format would generate the big exposing, and then I could whittle that down into my stable interface. Otherwise elm-format would leave it alone, so that

module Staff exposing (..)

wouldn't change. Or maybe vice versa?

If we must generate a huge 'exposing' clause, it would be nice to remove functions from it when they are renamed. It would be better to actually rename them, but that's probably hard to do reliably.

Trying to debug at the repl

The other issue I ran into was that I was trying to debug a module at the repl - one which has docs. I wasn't able to add extra functions to the exposing because elm-format rewrites the exposing clause to only contain functions from the docs each time.

Warry commented 5 years ago

Maybe it could be activated only on packages (from the elm.json), and leave them alone for applications.

jinjor commented 5 years ago

This feature can be problematic in test modules.

Related topic in discourse: https://discourse.elm-lang.org/t/tests-and-elm-format-overwriting-the-exposing-list/2353

Warry commented 5 years ago

@jinjor I honestly fail to see why?

It seems to me that this feature is meant to be used in a workflow that is not available yet (awaiting for compiler and possibly editor plugins updates). Hence my question is: is it a technical issue that blocks having this feature as opt in in the mean time?

jinjor commented 5 years ago

@Warry Sorry for the confusion. I meant, "this feature" is the current behavior. Not the one which is proposed here. (See the discourse thread. I cannot describe the problem better than that.)

brendan-jefferis commented 5 years ago

I'd prefer it if this feature was removed (or just used for packages if that's preferable for package authors)

exposing (..) is a valid language feature and I think it should be supported. I get that it's not best practice to expose all in most scenarios but I don't think the formatter should decide that for me.

mitchellwrosen commented 5 years ago

I would like to see this feature removed as well. It's very cumbersome during the initial rapid development/refactoring phase of building an application - I want to exposing everything for convenience.

mitchellwrosen commented 5 years ago

I'm using a workaround in the meantime.

Here's a patch:

diff --git a/src/ElmFormat/Render/Box.hs b/src/ElmFormat/Render/Box.hs
index 927a432..03e2b39 100644
--- a/src/ElmFormat/Render/Box.hs
+++ b/src/ElmFormat/Render/Box.hs
@@ -278,10 +278,15 @@ formatModuleHeader elmVersion modu =
       detailedListingIsMultiline _ = False

       varsToExpose =
-          sortVars
-              (detailedListingIsMultiline exportsList)
-              (detailedListingToSet definedVars exportsList)
-              documentedVars
+          case exportsList of
+              AST.Variable.OpenListing _ ->
+                  ([], [])
+
+              _ ->
+                  sortVars
+                      (detailedListingIsMultiline exportsList)
+                      (detailedListingToSet definedVars exportsList)
+                      documentedVars

       extractVarName :: TopLevelStructure Declaration -> [AST.Variable.Value]
       extractVarName decl =

And to apply it, save it as a file named exposing.patch, then

git checkout 68f0fc0b595ceee32bf94538154d4cf7766817f6
git apply exposing.patch
stack install
mitchellwrosen commented 5 years ago

@avh4 Would you accept the above patch as a PR?

andys8 commented 5 years ago

@avh4 I'm also interested in having a PR to change the behavior. Will it be merged or rejected?

jjant commented 5 years ago

@avh4 Could you give an answer? This issue has been open for over 10 months.

jfmengels commented 5 years ago

It looks like this problem has been addressed in a different issue. See https://github.com/avh4/elm-format/issues/578 for more details.