Skip to content
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

Call ignoreFile.IsIgnored with absolute path. #2656

Merged
merged 3 commits into from
Dec 22, 2022

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Dec 20, 2022

I discovered that ignored files can potentially be formatted by Fantomas.Client.
Steps to reproduce:

#r "nuget: Fantomas.Client"

open System
open System.IO
open Fantomas.Client.Contracts
open Fantomas.Client.LSPFantomasService

let service = new LSPFantomasService() :> FantomasService

let file = @"C:\Users\nojaf\Projects\fsharp\src\Compiler\Checking\NicePrint.fs"
let content = File.ReadAllText file

let formatReq : FormatDocumentRequest = {
    SourceCode = content
    FilePath = file
    Config = None
}

service.FormatDocumentAsync(formatReq).Result.Code

The code will read 1 which means Formatted, you should receive 4 (Ignored) instead.

I believe I found a fix for this problem but I would like a review from @Smaug123 to be sure.

@nojaf nojaf requested a review from Smaug123 December 20, 2022 16:23
fullPath

ignoreFile.IsIgnored path
ignoreFile.IsIgnored fullPath
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Smaug123 I believe the problem of using a relative path here is the following:
In loadIgnoreList we process the path with

let relativePath =
fs
.Path
.GetRelativePath(fs.Directory.GetParent(ignoreFilePath).FullName, path)
.Replace("\\", "/")

Looking at the docs, they mention GetFullPath is invoked before calculating the difference.

In the case of the cli tool, the relative will be resolved with the pwd because that is where we assume the ignore file lives. For the daemon, on the other hand, the pwd could be something really random and thus the difference is calculated wrongly.

Does this make sense to you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh, yeah, that does make sense. But might it be better to only pass fully-qualified paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But might it be better to only pass fully-qualified paths?

Something like this?

Copy link
Contributor

@Smaug123 Smaug123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems pretty plausible to me, although I can't say I am 100% confident.

@nojaf
Copy link
Contributor Author

nojaf commented Dec 22, 2022

Thanks @Smaug123, I think this will be fine to try out.
As I've reported the issue myself, I don't think many people encounter this problem in the wild.

@nojaf nojaf merged commit 1394b87 into fsprojects:main Dec 22, 2022
@Smaug123
Copy link
Contributor

Yep, sounds sane to me!

@nojaf nojaf deleted the ignore-file-daemon branch January 30, 2023 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants