[WIP] Fake.IO.FileSystem rework #1670

devcrafting · 2017-09-22T12:13:34Z

The idea was to add Obsolete documentation at first. I also splitted modules in their own files.

Then I noted that there is an issue of naming/casing consistency accross the FAKE API. Then, I also propose to use a consistent.

For now, I applied the rework only to DirectoryInfo module (given even without the casing changes, there are already some changes in the API). Wait for your feedback before I complete other modules of Fake.IO.FileSystem.

matthid

Thanks for taking care of the cleanup work!
There are some things I have not decided yet, but I think we can release it like this anyway (it probably is a bit more sane than before)

matthid · 2017-09-22T15:04:47Z

src/app/Fake.Core.Process/Process.fs

@@ -361,7 +361,7 @@ let tryFindFile dirs file =
                   |> String.replace "[ProgramFiles]" Environment.ProgramFiles
                   |> String.replace "[ProgramFilesX86]" Environment.ProgramFilesX86
                   |> String.replace "[SystemRoot]" Environment.SystemRoot
-                   |> DirectoryInfo.ofPath


I'm really back and forth about this. We have ofSeq ofList and a lot of these everywhere. But you are right in that this meets the F# Guidelines better :/

As said on Gitter, I asked people at FableConf, people seems to prefer camelCase for functions.

matthid · 2017-09-22T15:13:26Z

src/app/Fake.IO.FileSystem/Directory.fs

@@ -0,0 +1,49 @@
+namespace Fake.IO.FileSystem


Maybe these modules should move to Fake.IO :/

no preferences...

Yes I think we should move them (I can do that if you want), because then it is more similar to System.IO and one less open Fake.IO.FileSystem at the top...

matthid · 2017-09-22T15:14:05Z

src/app/Fake.IO.FileSystem/Directory.fs

+module Directory =
+
+    /// Creates a directory if it does not exist.
+    let CreateDir path = 


Yep not yet cleaned all modules, just the DirectoryInfo one.

matthid · 2017-09-22T15:15:39Z

src/app/Fake.IO.FileSystem/Directory.fs

+        if not (Directory.Exists dir) then
+            Directory.CreateDirectory dir |> ignore
+
+    let isDirectory path = Path.isDirectory path


Directory.isDirectory path reads a bit strange. Though I have no suggestion how to improve. Maybe just leave Directory.Exist and drop this?

Obsolete to use Path.IsDirectory?

matthid · 2017-09-22T15:17:18Z

src/app/Fake.IO.FileSystem/Directory.fs

+        |> DirectoryInfo.OfPath
+        |> DirectoryInfo.MatchingFiles pattern
+        |> fun files -> 
+            if Seq.isEmpty files then None


Seq.tryHead?

matthid · 2017-09-22T15:19:24Z

src/app/Fake.IO.FileSystem/Directory.fs

+            //|> (SetReadOnly false)
+            //logfn "Deleting %s" dir.FullName
+            dir.Delete true
+        else () //TODO: logfn "%s does not exist." dir.FullName


After all these comments: The Directory module is really not well thought through - for historic reasons. Read my comments not as something you need to fix but as thoughts I'm having when I go through the code...

matthid · 2017-09-22T15:20:05Z

src/app/Fake.IO.FileSystem/DirectoryInfo.fs

+         |> Seq.toList) @ files
+
+    /// Copies the file structure recursively.
+    let CopyRecursiveTo overwrite (outputDir : DirectoryInfo) (dir : DirectoryInfo) = CopyRecursiveToWithFilter overwrite (fun _ _ -> true) outputDir dir


The DirectoryInfo module reads a lot better...

matthid · 2017-09-22T15:26:00Z

src/app/Fake.IO.FileSystem/FileSystem.fs

@@ -1,304 +0,0 @@
-/// Contains helpers which allow to interact with the file system.
-namespace Fake.IO.FileSystem


Generally I don't feel we need to have a separate file for each module (this is not C#).

I'm not against splitting on some defined boundaries, for example we could split out the Operators or File/FileInfo, but as they are quite coupled I don't think that is too viable.

This module is probably one of the hardest to get into a nice state.

matthid · 2017-09-22T15:28:09Z

src/app/Fake.IO.FileSystem/Templates.fs

@@ -0,0 +1,26 @@
+/// NOTE: Maybe this should be an extra module?
+/// Contains basic templating functions. Used in other helpers.
+module Fake.IO.FileSystem.Templates


Yes, currently this is quite coupled throughout other modules. I don't like this module ... Kind of broken by design.

But works in practice /shrug

matthid · 2017-09-22T15:41:05Z

src/Fake-netcore.sln

 Microsoft Visual Studio Solution File, Format Version 12.00
 # Visual Studio 15
-VisualStudioVersion = 15.0.26124.0


I'm pretty sure something in here makes the build fail on travis... Maybe try to minimize the diff here or revert this change and add changes by hand?

The appveyor failure has something to do with the tests for the legacy FakeLib component. Some behavior was changed?

yep not intended to cmmit this file...I will revert

…ue ?!)

…e file)

… DirectoryInfo)

matthid

I really really like it :)

matthid · 2017-09-27T16:18:50Z

build.fsx

@@ -134,7 +139,7 @@ Target.Create "RenameFSharpCompilerService" (fun _ ->
    for framework in ["netstandard1.6"; "net45"] do
      let dir = __SOURCE_DIRECTORY__ </> "packages"</>packDir</>"lib"</>framework
      let targetFile = dir </>  "FAKE.FSharp.Compiler.Service.dll"
-      File.DeleteFile targetFile
+      File.Delete targetFile


This is System.IO.File.Delete isn't it?
Should we provide delete (ie lower case version) to have unified looking code?

I think there is no easy answer.

yes that's it. I saw System.IO.File is used a lot because some functions in Fake.IO.FileSystem modules are just one line call to BCL...
I will replace it in build.fsx

matthid · 2017-09-27T16:19:01Z

build.fsx

@@ -120,7 +125,7 @@ Target.Create "Clean" (fun _ ->
    //-- "src/*/*/obj/*.props"
    //-- "src/*/*/obj/*.paket.references.cached"
    //-- "src/*/*/obj/*.NuGet.Config"
-    |> File.DeleteFiles
+    |> File.deleteAll


good choice.

matthid · 2017-09-27T16:20:44Z

src/app/Fake.IO.FileSystem/Directory.fs

@@ -0,0 +1,49 @@
+namespace Fake.IO.FileSystem


Yes I think we should move them (I can do that if you want), because then it is more similar to System.IO and one less open Fake.IO.FileSystem at the top...

matthid · 2017-09-27T16:22:51Z

src/app/Fake.IO.FileSystem/Operators.fs

+
+open System.IO
+
+module Operators =


If we move to Fake.IO I'm not sure how we should call this. Fake.IO.Operators might be too generic. Maybe we leave it in this namespace or call it PathOperators?

I will change namespace to File.IO. And for Operators, PathOperators seems good ? or FileSystemOperators ? (since we are in Fake.IO.FileSystem package)

matthid · 2017-10-01T21:28:38Z

Ok I'm releasing this (Will move move everything to Fake.IO as well)

matthid reviewed Sep 22, 2017

View reviewed changes

devcrafting mentioned this pull request Sep 25, 2017

Removing "open Fake" results in "Shell is not defined" #1680

Closed

devcrafting force-pushed the feature/io-filesystem-rework branch 2 times, most recently from 671e441 to 429a520 Compare September 25, 2017 20:36

devcrafting added 8 commits September 26, 2017 19:59

Split FileUtils in two files and remove allFiles function (returns tr…

374d314

…ue ?!)

Separate modules FileInfo, FileSystemInfo, Operators

09cff4c

Adjust fsproj and remove FileFilter use

42e4ddd

Split Directory module in its own file

b204d63

Split DirectoryInfo into its own file, leaving File alone (then renam…

76c584d

…e file)

Add Obsolete for all DirectoryInfo module functions

5cfe171

Propose additional design guidelines on consistent naming/casing

beb7e53

Normalize and add Obsolete for Directory module

9eac867

devcrafting force-pushed the feature/io-filesystem-rework branch from 1838e79 to 9eac867 Compare September 26, 2017 18:00

devcrafting added 5 commits September 26, 2017 21:55

Fix tests (switched 2 parameters when removing duplicate functions in…

8be5e66

… DirectoryInfo)

Normalize naming and add Obsolete for File module

20f7d25

Fix build waiting releas of breaking changes

cce4762

Fix failed build on BootstrapTest

8d293b8

Use new API in build.fsx

13a8c33

matthid approved these changes Sep 27, 2017

View reviewed changes

matthid merged commit 13a8c33 into fsprojects:master Oct 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Fake.IO.FileSystem rework #1670

[WIP] Fake.IO.FileSystem rework #1670

devcrafting commented Sep 22, 2017

matthid left a comment

matthid Sep 22, 2017

devcrafting Sep 25, 2017

matthid Sep 22, 2017

devcrafting Sep 25, 2017

matthid Sep 27, 2017

matthid Sep 22, 2017

devcrafting Sep 22, 2017

matthid Sep 22, 2017

devcrafting Sep 22, 2017

matthid Sep 22, 2017

matthid Sep 22, 2017

matthid Sep 22, 2017

matthid Sep 22, 2017

matthid Sep 22, 2017

matthid Sep 22, 2017

matthid Sep 22, 2017

devcrafting Sep 22, 2017

matthid left a comment

matthid Sep 27, 2017

devcrafting Sep 28, 2017

matthid Sep 27, 2017

matthid Sep 27, 2017

matthid Sep 27, 2017

devcrafting Sep 28, 2017

matthid commented Oct 1, 2017

		@@ -1,304 +0,0 @@
		/// Contains helpers which allow to interact with the file system.
		namespace Fake.IO.FileSystem

[WIP] Fake.IO.FileSystem rework #1670

[WIP] Fake.IO.FileSystem rework #1670

Conversation

devcrafting commented Sep 22, 2017

matthid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthid commented Oct 1, 2017