New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to dotnet core #236

Merged
merged 31 commits into from May 2, 2018

Conversation

Projects
None yet
5 participants
@nojaf
Copy link
Collaborator

nojaf commented Mar 22, 2018

We hope to get more people involved in this project by migrating it to dotnet core 2.0.
All projects currently build and you can run the tests (but 5 fail).

Todo:

  • Create new dotnet core projects and reference existing files
  • Update all paket dependencies to latest
  • Fix failing unit tests *
  • Add support for full framework
  • Update build.fsx script (Fake 5 stable?)
  • Setup continuous integration
  • Change Cmd.Core to be a clitool, so that dotnet fantomas works
  • Remove the old projects

** unit test output

PS C:\Users\nojaf\Projects\fantomas\src\Fantomas.Tests.Core> dotnet test .\Fantomas.Tests.Core.fsproj
Build started, please wait...
C:\Users\nojaf\Projects\fantomas\src\Fantomas.Tests\SpecialConstructsTests.fs(16,4): warning FS0988: Main module of program is empty: nothing will happen when it is run [C:\Users\nojaf\Projects\fantomas\src\Fantomas.Tests.Core\Fantomas.Tests.Core.fsproj]
Build completed.

Test run for C:\Users\nojaf\Projects\fantomas\src\Fantomas.Tests.Core\bin\Debug\netcoreapp2.0\Fantomas.Tests.Core.dll(.NETCoreApp,Version=v2.0)
Microsoft (R) Test Execution Command Line Tool Version 15.3.0-preview-20170628-02
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
NUnit Adapter 3.10.0.21: Test execution started
Running all tests in C:\Users\nojaf\Projects\fantomas\src\Fantomas.Tests.Core\bin\Debug\netcoreapp2.0\Fantomas.Tests.Core.dll
NUnit3TestExecutor converted 285 of 285 NUnit test cases
Skipped  should align mis-aligned comments
Error Message:
 reason

Skipped  should keep comments on else if
Error Message:
 reason

NUnit Adapter 3.10.0.21: Test execution complete

Total tests: 285. Passed: 283. Failed: 0. Skipped: 2.
Test Run Successful.
Test execution time: 7.5011 Seconds

Please let me know if I'm missing anything.

@nojaf

This comment has been minimized.

Copy link
Collaborator Author

nojaf commented Mar 24, 2018

Hijacking this thread to talk about one of the failing tests:

/cc: @AnthonyLloyd

dotnet test .\Fantomas.Tests.Core.fsproj --filter "should keep compiler directives 2"

[<Test>]
let ``should keep compiler directives 2``() =
    formatSourceString false """
#if INTERACTIVE
#else
#load "../FSharpx.TypeProviders/SetupTesting.fsx"
SetupTesting.generateSetupScript __SOURCE_DIRECTORY__
#load "__setup__.fsx"
#endif
"""  config
    |> fun fss -> 
        printfn "%s" fss
        fss 
    |> should equal """
#if INTERACTIVE
#else
#load "../FSharpx.TypeProviders/SetupTesting.fsx"
SetupTesting.generateSetupScript __SOURCE_DIRECTORY__
#load "__setup__.fsx"
#endif
"""

The result of the printfn is:

 #if INTERACTIVE

 #else

 #load "../FSharpx.TypeProviders/SetupTesting.fsx"
 SetupTesting.generateSetupScript __SOURCE_DIRECTORY__
 #load "__setup__.fsx"
 #endif

 #load "../FSharpx.TypeProviders/SetupTesting.fsx"

 SetupTesting.generateSetupScript __SOURCE_DIRECTORY__

 #load "__setup__.fsx"

The piece inside the else block is printed twice. This is because the #if INTERACTIVE returns false and the else block is parsed by the FCS and is visible inside the ast.
To something like this

[SynModuleOrNamespace
   ([Program],false,true,
    [HashDirective
       (ParsedHashDirective
          ("load",["../FSharpx.TypeProviders/SetupTesting.fsx"],
           Program.fs (3,0--3,49) IsSynthetic=false),
        Program.fs (3,0--3,49) IsSynthetic=false);
     DoExpr
       (SequencePointAtBinding Program.fs (4,0--4,53) IsSynthetic=false,
        App
          (NonAtomic,false,
           LongIdent
             (false,
              LongIdentWithDots
                ([SetupTesting; generateSetupScript],
                 [Program.fs (4,12--4,13) IsSynthetic=false]),None,
              Program.fs (4,0--4,32) IsSynthetic=false),
           Const
             (String
                ("C:\Users\nojaf\Projects\fantomas\src",
                 Program.fs (4,33--4,53) IsSynthetic=false),
              Program.fs (4,33--4,53) IsSynthetic=false),
           Program.fs (4,0--4,53) IsSynthetic=false),
        Program.fs (4,0--4,53) IsSynthetic=false);
     HashDirective
       (ParsedHashDirective
          ("load",["__setup__.fsx"],Program.fs (5,0--5,21) IsSynthetic=false),
        Program.fs (5,0--5,21) IsSynthetic=false)],PreXmlDocEmpty,[],None,
    Program.fs (3,0--5,21) IsSynthetic=false)]

The fantomas context for this (created by Fantomas.FormatConfig.Context.create) is:

{Config = {IndentSpaceNum = 4;
            PageWidth = 80;
            SemicolonAtEndOfLine = false;
            SpaceBeforeArgument = true;
            SpaceBeforeColon = true;
            SpaceAfterComma = true;
            SpaceAfterSemicolon = true;
            IndentOnTryWith = false;
            ReorderOpenDeclaration = false;
            SpaceAroundDelimiter = true;
            StrictMode = false;};
  Writer = Fantomas.FormatConfig+ColumnIndentedTextWriter;
  BreakLines = true;
  BreakOn = <fun:get_Default@136>;
  Content =
   "
 #if INTERACTIVE
 #else
 #load "../FSharpx.TypeProviders/SetupTesting.fsx"
 SetupTesting.generateSetupScript __SOURCE_DIRECTORY__
 #load "__setup__.fsx"
 #endif
 ";
  Positions = [|0; 1; 17; 23; 73; 127; 149; 156; 157|];
  Comments = seq [];
  Directives = seq [];}

Because the content contains everything it kinda adds up to why we are seeing the the #else block twice. This behavior could be introduced due to the introduction of a newer FCS.

I checked with the original code to see if the output prints are the same or not.
And the AST contains the #if branch. Which is empty in this case.

ImplFile
  (ParsedImplFileInput
     ("/tmp.fsx",true,QualifiedNameOfFile Tmp$fsx,[],[],
      [SynModuleOrNamespace
         ([Tmp],false,true,[],PreXmlDocEmpty,[],null,
          /tmp.fsx (8,0--8,0) IsSynthetic=false)],(true, true)))

So the question is why the INTERACTIVE flag toggled? Might have something to do with using the FSharpParsingOptions instead of the old FSharpProjectOptions.

@nojaf

This comment has been minimized.

Copy link
Collaborator Author

nojaf commented Mar 24, 2018

Turns out it was a configuration problem indeed, unit tests are now fixed.

@nojaf nojaf referenced this pull request Mar 26, 2018

Closed

.NET Core 2.0 support #215

nojaf added some commits Mar 26, 2018

@danyx23

This comment has been minimized.

Copy link
Contributor

danyx23 commented Apr 21, 2018

Hi nojaf - this looks very promising, do you need help? I'd like to get Fantomas building/working under .net core and then start contributing small fixes. I saw that the main program executable is not yet implemented again, what are your plans here and can I help somehow?

@nojaf

This comment has been minimized.

Copy link
Collaborator Author

nojaf commented Apr 21, 2018

Hi @danyx23 , you could try the artifacts on AppVeyor. That part is almost done, we are facing some warnings on the full framework side of things.
Basically it should work if you download the artifacts and try installing fantomas as a clitool.

Add <add key="fantomas-core" value="C:\YourFolderWithLocalPackages" />
to "C:\Users\You\AppData\Roaming\NuGet\NuGet.Config"

Then in a fsproj you can add <DotNetCliToolReference Include="dotnet-fantomas" Version="2.7.0" />. Do a dotnet restore and try running dotnet fantomas "myfile.fs".

If you have any ideas regarding the full framework warnings, we are all ears.

AnthonyLloyd and others added some commits Apr 22, 2018

Merge pull request #1 from AnthonyLloyd/master
use net45 which is what FSharp.Compiler.Service references

nojaf added some commits Apr 25, 2018

@nojaf nojaf changed the title [WIP] Migrate to dotnet core Migrate to dotnet core Apr 25, 2018

@nojaf

This comment has been minimized.

Copy link
Collaborator Author

nojaf commented Apr 25, 2018

@dungpa could you please review this? We think it is finished and can be released.

AnthonyLloyd and others added some commits May 1, 2018

@dungpa

dungpa approved these changes May 2, 2018

Copy link
Member

dungpa left a comment

Wow. Great work!

@dungpa dungpa merged commit a97e521 into fsprojects:master May 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nojaf

This comment has been minimized.

Copy link
Collaborator Author

nojaf commented May 2, 2018

@dungpa many thanks for the merge! Let us know if it needs any more changes in order to be published on nuget.

@dungpa

This comment has been minimized.

Copy link
Member

dungpa commented May 2, 2018

@nojaf I have some problems with NuGet keys (couldn't push new packages).

Could I add you as a NuGet owner for pushing packages? If you agree, please give me your NuGet user name. Thanks.

@nojaf

This comment has been minimized.

Copy link
Collaborator Author

nojaf commented May 2, 2018

Thanks, my name is also nojaf.

@dungpa

This comment has been minimized.

Copy link
Member

dungpa commented May 2, 2018

I sent the owner request. Thanks again both.

@ErikSchierboom

This comment has been minimized.

Copy link

ErikSchierboom commented May 3, 2018

Great work! Any chance that the docs will be updated too? :)

@nojaf

This comment has been minimized.

Copy link
Collaborator Author

nojaf commented May 3, 2018

I'm planning to do this once it is all ok on NuGet, please try https://www.nuget.org/packages/dotnet-fantomas/2.7.1 and let me know. I'm also working on blog post regarding these changes.

@ErikSchierboom

This comment has been minimized.

Copy link

ErikSchierboom commented May 3, 2018

@nojaf I've just tested it, working fine! Thanks!

@danyx23

This comment has been minimized.

Copy link
Contributor

danyx23 commented May 3, 2018

Very cool, thank you all for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment