Auto-detect elm version per file #653

rlefevre · 2019-11-25T00:28:44Z

Resolves #561

rlefevre · 2019-11-25T10:24:52Z

To avoid unexpected behavior changes:

Per file detection is used only:
- when elm-format does not run with --upgrade
- when there is no --elm-version flag
Auto-detection from the directory where elm-format is run is still used to determine version with --stdin if no --elm-version flag is provided.

rlefevre · 2019-11-25T10:25:51Z

src/ElmFormat.hs

+                return $ Right $ concat $ files
+

+type ElmFile


Would you prefer a newtype instead? Or no type at all and just using the tuple everywhere?

Yeah, I think either newtype or data with named record fields is preferable.

Ok, I used a data with named record fields.

src/ElmFormat.hs

rlefevre · 2019-11-25T10:29:34Z

src/ElmFormat/Filesystem.hs

 hasFilename name path =
    name == FilePath.takeFileName path
+
+addElmVersion :: FileStore f => FilePath -> Free f (FilePath, ElmVersion)


You might prefer other names for this function and ElmVersion.fromFile, I'm open to suggestions if you don't like them.

Yeah, the name seems good. The one thing I wonder about is how it might work out to have findAllElmFiles return [ElmFile] instead of [FilePath] and having to use addElmVersion later.

Also having findAllElmFiles return [ElmFile] would allow a future optimization of detecting the version at the top and know it already as you recurse down into directories instead of later having to recurse up from each file to find its version.

I added the optimization in the last version.

src/ElmVersion.hs

rlefevre · 2019-11-25T16:12:38Z

The following instructions from README.md should also be updated:

You must run elm-format from the directory that contains your elm.json (for Elm 0.19) or elm-package.json (for Elm 0.18), or else you must pass the appropriate --elm-version=0.19/--elm-version=0.18 command line argument.

Maybe you have a suggestion or this can be done later when preparing the release?

avh4 · 2019-11-27T03:30:45Z

The following instructions from README.md should also be updated

I guess if we're not doing that now (because we want master's README to still describe the existing version, could you maybe make a new github issue to track that, and I'll put it at the end of the next milestone

avh4 · 2019-11-27T03:35:04Z

src/ElmFormat.hs

                do
                    let autoYes = Flags._yes config
                    resolvedInputFiles <- Execute.run (Execute.forHuman autoYes) $ resolveFiles (Flags._input config)
+                    detectedElmVersion <- Execute.run (Execute.forHuman autoYes) $ ElmVersion.fromFile "."


Can this IO action fail? If so, we should maybe postpone running this until we know that detectedElmVersion will be used.

This is still needed in last version, but things have changed a lot, so let's discuss it again based on the new version.

avh4 · 2019-11-27T03:39:24Z

src/ElmFormat.hs

+doIt :: (InputConsole f, OutputConsole f, InfoFormatter f, FileStore f, FileWriter f) => ElmVersion -> Flags.Config -> WhatToDo -> Free f Bool
+doIt elmVersion config whatToDo =
+    let
+        getVersion fileDetectedElmVersion =


This feels like it's starting to get messy to me. I'm thinking that we should probably try removing the elmVersion param to doIt and pushing it into each of the WhatToDo constructors that need it, and then we should also resolve the actual versions for each file (this getVersion logic) before calling doIt. What do you think?

Ok, done.
Things could be even simpler if we get rid of global auto-detection and just use 0.19.
To be discussed, see my other comment about that.

avh4 · 2019-11-27T03:49:33Z

src/ElmVersion.hs

+                        _ -> Elm_0_18
+
+            _ | path == dir ->
+                return Elm_0_19


I think this branch means that if there is not elm-package.json, and we've recursed out as far as we can, then give Elm_0_19. Is that right? I think it should probably return Nothing in this case and let the caller decide what to default to rather than hardcoding the default in this function -- is there anything I'm missing about this?

Oh, was this done to only have to check one file per level as you move outward?

Fixed in last version.

avh4 · 2019-11-27T03:55:48Z

src/ElmVersion.hs

+    do
+        let dir = takeDirectory (path)
+        elmPackageJson <- stat (path </> "elm-package.json")
+        case elmPackageJson of


Since we are now recursing upward (and it looks like we normalize the path, so we'll recurse up all the way to the root of the filesystem?) so that means we might go up to a folder that contains an elm.json or elm-package.json that is owned by a different user and maybe not readable by the current user. Will stat still succeed in that case? Are there any possible permissions that might cause an error? (Maybe we reach a folder that we can't list? though I'm not sure that would be possible given we are in a folder that's inside it.) If there are possible IO errors from the stat due to files that would be outside of the current user's control, I think we ought to make sure we ignore them rather than crashing.

Ok. This first naive implementation has at least the merits of raising a few potential design issues, so let's slow down and go back to agreeing on what we want, as this does not seem absolutely clear from #561 comments only.

First, there does not seem to be an universal agreement of what normalize means for paths, but System.FilePath.normalise used in the PR does not make a path an absolute one. This can be noticed by the fact that it does not use some IO (Sytem.Path.toAbsoluteFilePath does). System.FilePath.takeDirectory does not make the path absolute either.

and it looks like we normalize the path, so we'll recurse up all the way to the root of the filesystem

We don't make the path an absolute path (the only use of normalise is in TestWorld.hs and this does not make a path absolute as explained previously), so we won't recurse all the way except if the path is already an absolute path.
takeDirectory is only a string manipulation function, it has not real knowledge of the filesystem.
Consequently, for example:

./src/file.elm will stop at .

/tmp/file/elm will stop at /

../test.elm will actually test .. then ., because maybe surprisingly takeDirectory ".." == "." (and takeDirectory "../.." == "..") 🤔

Therefore, if elm-format is run from a directory below the elm.json or elm-package.json file, the version may not be correctly detected.

For example if in a 0.18 package, elm-format . is run from the src sub-directory (instead of the package root directory with the elm-package.json inside), this will return the default value 0.19.

After your review, it seems to me that preferably, the design should be instead:

For each file argument:

make it absolute (which is actually also an IO operation that recursively walks upward up to /)

search for elm-package.json or elm.json upward up to /, trying to detect an upward version

then if it is a directory, continue recursively downward to search elm files, detecting elm-package.json and elm.json on the way, and therefore downward versions (in an optimized way), once per directory.

From a performance point of view, this would optimize the findAllElmFiles case (a directory argument), but would not optimize the detection of several files arguments, but I think the main use cases are a single file argument or a directory, so this could be acceptable?

A significant downside of this approach is that this would require to really simulate directories in TestWorld.hs, which seems quite daunting and error prone (also what is the directory above . in TestWorld?). An alternative would be to use an existing implementation like fakefs, which seems quite limited also, or to hack a very small working use case for what already exists and do real tests instead for more complicated cases. None of them seems completely compelling but a compromise would have to be found.

Current implementation is simple in TestWorld because we don't transform to absolute paths and only use string operations on paths, which does not require to add filesystem operations in FileStore.hs (only normalise to get rid of ./ prefixes and change / to \ on windows), however test cases should not dictate the design. Maybe I am just lazy 😅.

@avh4, what do you think?

avh4 · 2019-11-27T03:56:59Z

tests/Integration/CliTest.hs

+                |> TestWorld.uploadFile "0.19/src/test.elm" "module Main exposing (f)\n\n\nf =\n    '\\u{2000}'\n"
+                |> TestWorld.uploadFile "0.19/elm.json" "{\"elm-version\": \"0.19.0 <= v < 0.20.0\"}"
+                |> run "elm-format" ["0.18/src/test.elm", "0.19/src/test.elm", "--validate"]
+                |> expectExit 0


avh4 · 2019-11-27T03:57:30Z

Looks like a great start, thanks!

avh4 · 2019-11-28T04:40:25Z

Ah, well my comments were based on the assumption that normalise was getting to an absolute path. I'm not actually sure what's necessary...

There are three main uses of elm-format: use from editors and editor plugins, use from the command line, and use from CI. For editors, is it common for editors to be running elm-format from a working directory that's inside of the folder containing the elm.json? I think probably not, though maybe I'm wrong about that. For command line use, I can imagine someone might cd into a subfolder and want to run elm-format just on files under that folder. I think that would require going up through the actual filesystem and not just removing segments from the path. Is that a case we want to support? As for CI use, I don't think elm-format would typically be run from the project folder or from the folder where the elm.json is.

rlefevre · 2019-12-02T08:19:12Z

I made a new version that assumes that running elm-format from command line in a sub-directory of the Elm project should work. Consequently, elm version detection is done first upward, then downward during files finding.

For now I have kept the global current directory Elm version detection to avoid behavior regressions, but I'm really not sure it makes sense anymore. It seems that the only use case is running in an 0.18 project with --stdin and without --elm-version.

rlefevre · 2019-12-02T08:23:34Z

src/ElmFormat.hs

                do
                    let autoYes = Flags._yes config
-                    resolvedInputFiles <- Execute.run (Execute.forHuman autoYes) $ resolveFiles (Flags._input config)
+                    currentDirectoryElmVersion <- Execute.run (Execute.forHuman autoYes) $ FS.findElmVersion "."


I'm not sure this is useful anymore versus just using 0.19 as the default version.

I kept current working directory global version detection to avoid behavior regressions, but I fail to see valid use cases for it. The only one I see is running in a 0.18 project with --stdin and without --elm-version, is it worth it?

rlefevre · 2019-12-02T08:25:01Z

src/ElmFormat/Filesystem.hs

+    = ElmFile
+        { version :: ElmVersion
+        , path :: FilePath
+        } deriving (Show)


Just used deriving Show for my own private tests. It can be removed if you want.

rlefevre · 2019-12-02T08:27:14Z

src/ElmFormat/World.hs

-    doesFileExist = Dir.doesFileExist
-    doesDirectoryExist = Dir.doesDirectoryExist
+    doesFileExist path = do
+        exists <- try (Dir.doesFileExist path) :: IO (Either IOException Bool)


This is to avoid exceptions if detecting the version upward fails (because of permissions issues or something).

Maybe you would have preferred using try inside the Free Monad?
If so I would need guidance as I'm not sure how to do this with Free monad.

rlefevre · 2019-12-02T08:28:11Z

src/Messages/Formatter/HumanReadable.hs

-renderInfo (FileWouldChange file) =
-    putStrLn $ "File would be changed " ++ file
+renderInfo (FileWouldChange file version) =
+    putStrLn $ "File would be changed " ++ file ++ " (" ++ show version ++ ")"


Is it ok to you? It seems useful to add the detected version in output.

rlefevre · 2019-12-02T08:29:04Z

tests/ElmFormat/TestWorld.hs

        return False

+    makeAbsolute path =
+        -- wrong but enough for tests


Is it ok to you? I'm not sure that writing a lot of code for this now is worth it.

rlefevre · 2019-12-02T09:42:08Z

src/ElmFormat.hs

+determineWhatToDoFromConfig config defaultElmVersion resolvedInputFiles =
    do
-        source <- determineSource (Flags._stdin config) resolvedInputFiles
+        checkUpgradeVersion (Flags._upgrade config) (Flags._elmVersion config)


In 0.8.2, upgrade can actually use the auto-detected current directory elm version, which seems confusing to me (ie: --upgrade is possible without --elm-version):

MustSpecifyVersionWithUpgrade says:

"I can only upgrade code to specific Elm versions. To make sure I'm doing what you expect, you must also specify --elm-version=" ++ show elmVersion ++ " when you use --upgrade."

This check makes sure that a flag is provided, whatever the result of "current directory elm version" (which might be removed) or "per file version detection" (which is overridden by the flag).

I'm not sure if this is the expected behavior.

Resolves avh4#561

rlefevre · 2019-12-02T09:52:46Z

src/ElmFormat.hs

+        (True, Elm_0_18) ->
+            Elm_0_18_Upgrade
+
+        (True, _) ->


checkUpgradeVersion makes actually impossible to use something else than 0.19.

Still, the whole upgrade version code (check and upgrade) seems a little messy, but I'm not sure of the exact behavior we want, nor how to refactor this to avoid splitting the version check and the version upgrade later.

rlefevre · 2019-12-06T10:49:21Z

src/ElmFormat/World.hs

+        return $ Either.fromRight False exists
+
    listDirectory = Dir.listDirectory
+    makeAbsolute = Dir.makeAbsolute


This might also fail but I'm not sure what to do or if we want to handle this 🤔

rlefevre force-pushed the autodetect-elm-version-per-file branch 5 times, most recently from b54dae9 to 324b2a2 Compare November 25, 2019 10:19

rlefevre marked this pull request as ready for review November 25, 2019 10:24

rlefevre commented Nov 25, 2019

View reviewed changes

rlefevre force-pushed the autodetect-elm-version-per-file branch from 324b2a2 to 15f3458 Compare November 26, 2019 00:28

rlefevre changed the title ~~Autodetect elm version per file~~ Auto-detect elm version per file Nov 26, 2019

avh4 reviewed Nov 27, 2019

View reviewed changes

avh4 added this to the 0.8.4 milestone Nov 27, 2019

rlefevre force-pushed the autodetect-elm-version-per-file branch from 15f3458 to 3b2edf7 Compare December 2, 2019 08:12

rlefevre force-pushed the autodetect-elm-version-per-file branch from 3b2edf7 to 8addbf1 Compare December 2, 2019 08:21

rlefevre commented Dec 2, 2019

View reviewed changes

rlefevre mentioned this pull request Dec 2, 2019

Update README once "per-file detection" is merged and before release #655

Open

rlefevre force-pushed the autodetect-elm-version-per-file branch 2 times, most recently from f8a2339 to 244172d Compare December 2, 2019 09:35

rlefevre commented Dec 2, 2019

View reviewed changes

Auto-detect elm version per file

c09e060

Resolves avh4#561

rlefevre force-pushed the autodetect-elm-version-per-file branch from 244172d to c09e060 Compare December 2, 2019 09:50

rlefevre commented Dec 2, 2019

View reviewed changes

rlefevre commented Dec 6, 2019

View reviewed changes

avh4 modified the milestones: 0.8.4, 0.8.5 Sep 14, 2020

avh4 changed the base branch from master to main September 18, 2020 02:54

avh4 modified the milestones: 0.8.5, 0.8.6 Feb 4, 2021

avh4 modified the milestones: 0.8.6, Future 0.8.* Jan 26, 2023

Auto-detect elm version per file #653

Are you sure you want to change the base?

Auto-detect elm version per file #653

Uh oh!

Conversation

rlefevre commented Nov 25, 2019

Uh oh!

rlefevre commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rlefevre commented Nov 25, 2019

Uh oh!

avh4 commented Nov 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlefevre Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avh4 commented Nov 27, 2019

Uh oh!

avh4 commented Nov 28, 2019

Uh oh!

rlefevre commented Dec 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlefevre Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

rlefevre commented Nov 25, 2019 •

edited

Loading

rlefevre Nov 27, 2019 •

edited

Loading

rlefevre Dec 2, 2019 •

edited

Loading