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.

Sign up for GitHub

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

Jump to bottom

Only load the .fantomasignore file once, except for the daemon #2097

Merged
merged 12 commits into from
Mar 27, 2022
Merged

Only load the .fantomasignore file once, except for the daemon #2097

merged 12 commits into from
Mar 27, 2022

Conversation

Smaug123
Copy link
Contributor

@Smaug123 Smaug123 commented Feb 14, 2022 •
edited
Loading

A simpler attempt at #2086.

Note that this is a breaking change in behaviour; previously we would traverse the filesystem to find a .fantomasignore, and now we do not.

nojaf
nojaf approved these changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Hi again Patrick, thanks for this second attempt. It is a bit less impactful I think.
You are right that the functionality is somewhat breaking, but I think I changed it without much thought in 4.6. I'm not sure anyone will really notice, to be honest.

@Smaug123
Copy link
Contributor Author

(Got some yaks to shave here, sorry - FAKE's logic for finding reference assemblies is borked if your dotnet is in a custom location, as it is when installed through Nix.)

@nojaf
Copy link
Contributor

nojaf commented Feb 27, 2022

That one rings a bell actually, I got around a problem some time ago by setting FAKE_SDK_RESOLVER_CUSTOM_DOTNET_PATH.

@Smaug123
Copy link
Contributor Author

That looks plausible as a workaround, thanks; I've submitted a fix for the underlying issue as fsprojects/FAKE#2662

@Smaug123
Copy link
Contributor Author

Smaug123 commented Feb 27, 2022 •
edited
Loading

(The bump to .NET 6 has fixed a bunch of the places where I needed to do strange things to get Fantomas building, but sadly it appears to have introduced some new ones :/ no problems with Fantomas, I think, but I've hit a number of bugs in FAKE.)

@Smaug123
Copy link
Contributor Author

I'm going to park working on this until dotnet/runtime#64266 is released (probably .NET 6.0.3) and FAKE starts working again on darwin-aarch64.

@nojaf
Copy link
Contributor

nojaf commented Feb 28, 2022

Ok, no worries. I might take a look at this myself when I find some time.

@Smaug123
Copy link
Contributor Author

@nojaf I found a Windows machine! So I fixed up the test; do you want to have a quick look again? The fix was 11ae382 .

nojaf
nojaf requested changes Mar 27, 2022
View reviewed changes
Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

This looks very good! A have a couple of concerns but nothing major.
Many thanks for picking this one up!

src/Fantomas/paket.references Show resolved Hide resolved
paket.dependencies Outdated Show resolved Hide resolved
nojaf
nojaf approved these changes Mar 27, 2022
View reviewed changes
Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Thank you!

@nojaf nojaf merged commit e76fd06 into fsprojects:master Mar 27, 2022
@nojaf
Copy link
Contributor

nojaf commented Mar 27, 2022

Published as https://www.nuget.org/packages/fantomas-tool/4.7.5

jindraivanek pushed a commit to jindraivanek/fantomas that referenced this pull request Mar 30, 2022
@Smaug123 @jindraivanek
Only load the .fantomasignore file once, except for the daemon (fspro…
499bb63 
…jects#2097)

* Only load the .fantomasignore file once, except for the daemon

* Update documentation

* Also traverse upwards in default case

* Make comment less confusing

* Format

* Fix logic to relativise to ignorefile location

* Refactor to add unit tests

* Fix relative path logic again

* Move references to minimal locations

* Update Documentation.md after re-adding search
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nojaf nojaf nojaf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Smaug123 @nojaf