FakeVar enhancements #1978

BlythMeister · 2018-06-04T10:28:35Z

Added:

Get with type
Get or default
Get or fail

Also added tests to the above

matthid · 2018-06-04T10:53:27Z

src/app/Fake.Core.Context/Context.fs

  forceFakeContext()
  |> getFakeContext name
  |> Option.map (fun o -> o :?> 'a)
+
+let getFakeVarOrFail<'a> name =
+  match getFakeVar<'a> name with


I'd prefer this to be in a new module and try to keep the api surface (and changes) of the context module as minimal as possible. This is the only module where you have to update the runner in order to get the changes. Therefore your suggestion will break on old runners

Adding the generic parameter is (I believe) harmless.

ohhh really :s

i wonder if it's possible to remove all the variable stuff?

i wonder if it's possible to remove all the variable stuff?

We maybe could move it somewhere else but have to leave an obsolete attribute for now (we can do the breaking change sometime after the release)

Exactly what I've started working on 😂
All internal usage I'll migrate away from obsolete.

This makes it harder for newbies to understand since the core readme on expecto states "All expect-functions have the signature actual -> expected -> string -> unit, leaving out expected when obvious from the function." When flip is needed (for pipelines) it's explicitly mentioned

BlythMeister · 2018-06-04T11:45:03Z

Have also updated use of Expecto.Flip as this is quite confusing and subtle when used as a file level "open" statement

matthid · 2018-06-04T11:57:19Z

Looking at this, we probably should have basic integration-tests to test if we are still compatible with the old runner

BlythMeister · 2018-06-04T12:00:22Z

Agreed...not sure how to implement...

matthid · 2018-06-04T12:11:27Z

not sure how to implement...

The tests? Basically like the current integration tests, but using the "installed" version if fake and not the built one (for example we could use dotnet fake)

Also remove the obsolete i added on AssemblyInfoFile - if someone wants to directly call, they should be able to...

BlythMeister · 2018-06-04T14:01:39Z

@matthid done the refactoring and also fixed a few other things which were build warnings to make it easier to see the wood for the trees when building locally

Improved interface for remove and set variables

matthid · 2018-06-04T16:42:16Z

src/app/Fake.Core.Variables/Variables.fs

+  |> Option.map (fun o -> try
+                            o :?> 'a
+                          with e -> 
+                            failwithf "Variable '%s' - %s" name e.Message


please use raise <| exn(sprintf "", e) instead (at the very least)

matthid · 2018-06-04T16:45:48Z

src/app/Fake.Core.Variables/Variables.fs

@@ -21,29 +25,35 @@ let getOrDefault<'a> name defaultValue =
 let remove name =
  forceFakeContext()
  |> removeFakeContext name
-  |> Option.map (fun o -> o :?> 'a)
+  |> ignore


Sometimes you might want to know which value has been removed? Anyway I'm not sure if it is worth the potential breaking change ;) (On the other hand this one isn't too bad - I don't think anyone uses this directly at the moment.

I was working on this being brand new, so not breaking change.
Nobody is using this function at the moment 😉

Ah sorry, I'm probably a bit over cautious on this thing :)
Problem is this is the "connection" between runtime and script and breaking stuff there is a huge pain to cleanup :P

I completely agree, I was unaware of the link originally

matthid · 2018-06-04T16:50:30Z

src/app/Fake.DotNet.AssemblyInfoFile/AssemblyInfoFile.fs

@@ -213,7 +213,6 @@ module AssemblyInfoFile =

    /// Creates a C# AssemblyInfo file with the given attributes and configuration.
    /// The generated AssemblyInfo file contains an AssemblyVersionInformation class which can be used to retrieve the current version no. from inside of an assembly.
-    [<System.Obsolete "Please use 'create' instead">]


You don't like create anymore or what is the rationale?

I thought I was a big agressive on the obsolete. What if someone wanted to control themselves?
Since we don't plan to delete, only make private i see no harm in leaving without obsolete

Since we don't plan to delete

;)

matthid · 2018-06-04T16:54:47Z

Generally I really like the direction of this. Good idea to make this a thing and simplify the context module.

Have also updated use of Expecto.Flip as this is quite confusing and subtle when used as a file level "open" statement

I always thought that's how to use it :/. I personally don't really like the default signature, but whatever

matthid · 2018-06-04T16:57:01Z

src/app/Fake.Core.Variables/Variables.fs

@@ -0,0 +1,59 @@
+/// This module contains helpers for managing build time variables
+module Fake.Core.Variables


Should we add [<RequireQualifiedAccess>]?

BlythMeister · 2018-06-04T17:14:22Z

Re expecto I agree it seems wrong, but personnally spent an hour confused as the code didn't match the docs 😂

matthid · 2018-06-04T17:18:54Z

Seems like AppVeyor died :/

BlythMeister · 2018-06-04T17:20:49Z

I noticed it vanished... I didn't do it. 😂
Last commits made from phone as I'm away from laptop until the morning... I hope they compile!

matthid · 2018-06-04T18:08:25Z

src/app/Fake.Core.Variables/Variables.fs

@@ -0,0 +1,60 @@
+/// This module contains helpers for managing build time variables
+[<RequireQualifiedAccess>]
+module Fake.Core.Variables


I just fixed the compilation error and I'm not sure about the plural form. But Variable.set doesn't really sound better. Maybe explicitly call it FakeVar or FakeContext for example? Any idea?

I like FakeVar

Though that means there is a function called fakeVar so would be FakeVar.fakeVar

How about ContextVariable ?

Yes but FakeVar.fakeVar ;)
maybe FakeVar.create instead of that and
FakeContext.set for the raw stuff (or we make that internal)

Oooo FakeVar.create love it!

BlythMeister · 2018-06-04T19:24:48Z

@matthid made the updates to Fake.Core.FakeVar and really quite like it.
Went with "define" rather than "create" though :)

matthid

Yes it reads really nice now!

BlythMeister · 2018-06-04T19:41:04Z

Shame it has build error.
Will pick it up tomorrow 🤦🏼‍♂️

BlythMeister · 2018-06-05T15:05:19Z

matthid · 2018-06-05T15:27:57Z

src/app/Fake.Core.Tasks/Fake.Core.Tasks.fsproj

-    <ProjectReference Include="..\Fake.Core.Context\Fake.Core.Context.fsproj">
-      <FromP2P>true</FromP2P>
-    </ProjectReference>
+    <ProjectReference Include="..\Fake.Core.Context\Fake.Core.Context.fsproj" FromP2P="true" />


What does P2P mean here? I hope this has no effect on dependencies.

It was already here, i just made the projects nicer.
From what i can tell, it enables the transitive project dependencies to be pulled

I actually think all of the references should have it...based on this: https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-add-reference

P2P = project-to-project

matthid · 2018-06-05T15:28:26Z

src/app/Fake.Core.Trace/Fake.Core.Trace.fsproj

@@ -19,6 +19,7 @@
    <ProjectReference Include="..\Fake.Core.Environment\Fake.Core.Environment.fsproj" />
    <ProjectReference Include="..\Fake.Core.String\Fake.Core.String.fsproj" />
    <ProjectReference Include="..\Fake.Core.Context\Fake.Core.Context.fsproj" />


We probably could remove Fake.Core.Context now (but I guess it doesn't hurt)

matthid · 2018-06-05T15:30:18Z

Thanks a lot for going all the way with this :) Let's see how it works on staging

BlythMeister added 6 commits June 4, 2018 11:11

Add ability to get fake var with a type

b6f0a86

Add function to get fakevar or fail if not present

5fcfbac

Add get fakevar or default function

a6467ab

Added tests on FakeVar logic

0e0453e

Update to use the type passing on fakevar

d44b388

Build and test fix

1298eb0

matthid reviewed Jun 4, 2018

View reviewed changes

BlythMeister added 4 commits June 4, 2018 14:43

Migrate FakeVar usage into separate module

7f33f85

Fix AssemblyInfo warnings

73da350

Fix more warnings

d5e51e8

Fix some more warnings

3c44abb

Also remove the obsolete i added on AssemblyInfoFile - if someone wants to directly call, they should be able to...

BlythMeister added 2 commits June 4, 2018 15:36

Add the new file into FakeLib.fsproj

cdd194c

Added tests for type mismatch

b71163e

Improved interface for remove and set variables

matthid reviewed Jun 4, 2018

View reviewed changes

BlythMeister added 2 commits June 4, 2018 18:15

Add [<RequireQualifiedAccess>] to variables.fs

e1f8edd

Update error message on cast error

6dba481

Fix tests now error has changed

5882326

BlythMeister and others added 2 commits June 4, 2018 18:21

Improve cast error message

4a9867b

Fix compilation

d662d63

matthid reviewed Jun 4, 2018

View reviewed changes

Rename Fake.Core.Variables to Fake.Core.FakeVar

baca2d8

matthid approved these changes Jun 4, 2018

View reviewed changes

BlythMeister added 7 commits June 5, 2018 10:44

add vscode restore task & nest FakeVar project in Fake.sln

2b66f5a

Test fixes

1671636

Remove redundant statements

9448868

Correct FakeVar error message

7faa14c

Test fixes

e649774

Simplify and add P2P="true"

b9fb1c2

Fixing INT tests

99e7402

matthid reviewed Jun 5, 2018

View reviewed changes

matthid merged commit c6ba91c into fsprojects:release/rc Jun 5, 2018

BlythMeister deleted the fakeVarType branch May 11, 2021 15:58

		@@ -0,0 +1,59 @@
		/// This module contains helpers for managing build time variables
		module Fake.Core.Variables

FakeVar enhancements #1978

FakeVar enhancements #1978

Conversation

BlythMeister commented Jun 4, 2018

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

BlythMeister commented Jun 4, 2018

matthid commented Jun 4, 2018

BlythMeister commented Jun 4, 2018

matthid commented Jun 4, 2018

BlythMeister commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlythMeister Jun 4, 2018 • edited Loading

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 Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlythMeister commented Jun 4, 2018 • edited Loading

matthid commented Jun 4, 2018

BlythMeister commented Jun 4, 2018

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

BlythMeister commented Jun 4, 2018

matthid left a comment

Choose a reason for hiding this comment

BlythMeister commented Jun 4, 2018

BlythMeister commented Jun 5, 2018

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 Jun 5, 2018

BlythMeister Jun 4, 2018 •

edited

Loading

BlythMeister commented Jun 4, 2018 •

edited

Loading