search

fsprojects / FSharpLint

Lint tool for F#
https://fsprojects.github.io/FSharpLint/
MIT License
303 stars 73 forks source link

Create an API layer for external process #637

Open MrLuje opened 9 months ago

MrLuje commented 9 months ago

~## Warning, I branched from the NET 6 version, so https://github.com/fsprojects/FSharpLint/pull/606 should be merged first (ignore the first 4 commits for the review)~

WIP on https://github.com/fsprojects/FSharpLint/issues/627, it covers the following points:

Add --daemon flag to FSharpLint.Console All the "tool location detection" and "process handling" is copied from Fantomas project and adapted, basically:

~I'll rebase everything once https://github.com/fsprojects/FSharpLint/pull/606 is merged~

knocte commented 9 months ago

Good work @MrLuje!

I'll rebase everything once https://github.com/fsprojects/FSharpLint/pull/606 is merged

Thanks, I plan to re-review and/or merge that PR (or an equivalent) this week, bare with me.

MrLuje commented 9 months ago

Added an FSHARPLINT_SEARCH_PATH_OVERRIDE env var that will mostly be used for testing that can override search location (global tool & local tool) and a test to ensure FCS is not referenced by FSharpLint.Client

knocte commented 9 months ago

Hello @MrLuje sorry for the delay here. Today I merged the NET5->NET6 transition. Can you now rebase this PR? Thanks!

MrLuje commented 9 months ago

@knocte done

knocte commented 9 months ago

@MrLuje side-note, the conflict is actually caused because there is a change in the 1st commit of this PR that has some unnecessary whitespace noise hehe:

diff --git a/src/FSharpLint.Console/FSharpLint.Console.fsproj b/src/FSharpLint.Console/FSharpLint.Console.fsproj
index e70053133..f97b5e02b 100644
--- a/src/FSharpLint.Console/FSharpLint.Console.fsproj
+++ b/src/FSharpLint.Console/FSharpLint.Console.fsproj
@@ -1,9 +1,7 @@
-<Project Sdk="Microsoft.NET.Sdk">
-
+<Project Sdk="Microsoft.NET.Sdk">
   <PropertyGroup>
     <OutputType>Exe</OutputType>
     <TargetFrameworks>net6.0</TargetFrameworks>
-
     <Title>FSharpLint.Console</Title>
     <Description>Console application to run FSharpLint.</Description>
     <PackageTags>F#;fsharp;lint;FSharpLint;fslint;cli</PackageTags>
@@ -14,15 +12,15 @@
     <IsPackable>true</IsPackable>
     <RollForward>Major</RollForward>
   </PropertyGroup>
-
   <ItemGroup>
     <Compile Include="Output.fs" />
+    <Compile Include="Version.fs" />
+    <Compile Include="Daemon.fs" />
     <Compile Include="Program.fs" />
   </ItemGroup>
-
   <ItemGroup>
     <ProjectReference Include="..\FSharpLint.Core\FSharpLint.Core.fsproj" />
+    <ProjectReference Include="..\FSharpLint.Client\FSharpLint.Client.fsproj" />
   </ItemGroup>
-
   <Import Project="..\..\.paket\Paket.Restore.targets" />
-</Project>
+</Project>
\ No newline at end of file
knocte commented 8 months ago

@knocte done

Oh, interesting, CI failed on your fork (macOS job), but I can't see the log because I guess I don't have permissions. Can you share it?

MrLuje commented 8 months ago

@knocte done

Oh, interesting, CI failed on your fork (macOS job), but I can't see the log because I guess I don't have permissions. Can you share it?

Not a lot more information from my side (it timed out after 6h, maybe a runner issue ?) image

No more issue after a retry

knocte commented 8 months ago

@MrLuje sure, I'll do it this week. BTW can you rebase the PR? looks like there is some conflict

Hey @MrLuje, sorry for our delayed action on this, I just pushed a commit that introduces a File type, similar to Folder.

Now, there are a couple of things to fix here:

  1. Early on when you published this PR for the first time, I saw a potential improvement about some lines of code you wrote, but I realised that the "improvable code fragment" could actually be detected by FSharpLint itself, and in fact we had a rule in the works to detect this. This rule is now merged to master, so if you rebase this PR, you will see the offending code and CI will tell you what to change and how :)
  2. After working on the File type explained above, I've spotted another piece of code that is a bit confusing because it seems to be conflating files with folders, I will go ahead now (after I post this comment) to the "Files changed" tab and point it out there with more context.
MrLuje commented 8 months ago

@MrLuje sure, I'll do it this week. BTW can you rebase the PR? looks like there is some conflict

Hey @MrLuje, sorry for our delayed action on this, I just pushed a commit that introduces a File type, similar to Folder.

Now, there are a couple of things to fix here:

  1. Early on when you published this PR for the first time, I saw a potential improvement about some lines of code you wrote, but I realised that the "improvable code fragment" could actually be detected by FSharpLint itself, and in fact we had a rule in the works to detect this. This rule is now merged to master, so if you rebase this PR, you will see the offending code and CI will tell you what to change and how :)
  2. After working on the File type explained above, I've spotted another piece of code that is a bit confusing because it seems to be conflating files with folders, I will go ahead now (after I post this comment) to the "Files changed" tab and point it out there with more context.

Got it, cool new rule :)