Skip to content

Conversation

@progressive-galib
Copy link
Contributor

Description

"#if" directive around nullness removed fromsrc/Compiler/DependencyManager/DependencyProvider.fs and refactored.

Related: #18061 (partially addresses)

Checklist

  • Release notes entry updated: in
    docs/release-notes/.FSharp.Compiler.Service/9.0.200.md,

progressive-galib and others added 2 commits January 7, 2025 08:08
"#if" directive around nullness removed from src
Related: dotnet#18061 (partially addresses)

- [x] Release notes entry updated: in
    `docs/release-notes/.FSharp.Compiler.Service/9.0.200.md`,
@progressive-galib progressive-galib requested a review from a team as a code owner January 7, 2025 08:14
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2025

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@progressive-galib progressive-galib marked this pull request as draft January 7, 2025 16:02
@progressive-galib progressive-galib marked this pull request as ready for review January 7, 2025 16:32
@progressive-galib progressive-galib force-pushed the 18061-DependencyProvider branch from bfbca20 to cf575d3 Compare January 7, 2025 19:13
@T-Gro
Copy link
Member

T-Gro commented Jan 8, 2025

/run fantomas

@T-Gro
Copy link
Member

T-Gro commented Jan 8, 2025

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2025

@T-Gro
Copy link
Member

T-Gro commented Jan 8, 2025

Does "dotnet fantomas ." fail for you locally as well?
What if you manually try updating to a newer Fantomas version?
(if it is because of |null, it might need a tool update)

@Martin521
Copy link
Contributor

I guess the changed files must temporarily be put into the nullness section of .fantomasignore until fantomas 7 is out.

@psfinaki
Copy link
Contributor

psfinaki commented Jan 9, 2025

So I took a look:

  • dotnet fantomas . indeed fails locally here
  • the fantomas version we have is the latest available in our feeds
  • we could update the feeds and the fantomas version but the newest fantomas (6.3.16) just throws StackOverflow on our repo (even main)

(fyi @nojaf)

So I think @progressive-galib the best we can do here is to revert formatting changes (and also the ones which are deemed not necessary by @T-Gro) and put the DependencyProvider to the .fantomasignore.

Sorry for the inconvenience here, nulls shook the F# ecosystem a bit. Other files might cause less problems :)

@nojaf
Copy link
Contributor

nojaf commented Jan 9, 2025

Hi,

Could you try the latest beta maybe? That has initial support for all the nullness stuff: https://www.nuget.org/packages/fantomas/7.0.0-beta-001

You PR here will need to be ignored (via .fantomasignore) as it seems to have nullness syntax.

@psfinaki
Copy link
Contributor

psfinaki commented Jan 9, 2025

I think it behaves differently and does some changes, but eventually the beta also throws SO :/

@nojaf
Copy link
Contributor

nojaf commented Jan 10, 2025

@psfinaki could you share some more details? Latest beta worked for me on this branch.

image

@progressive-galib
Copy link
Contributor Author

progressive-galib commented Jan 11, 2025

@psfinaki could you share some more details? Latest beta worked for me on this branch.

image

could you push cause it fails on my machine.

@nojaf
Copy link
Contributor

nojaf commented Jan 11, 2025

You need to ignore the file for now.

@psfinaki
Copy link
Contributor

@progressive-galib sure, I pushed the changes to the PR.

@psfinaki
Copy link
Contributor

@nojaf that's what I'm getting in the main:
image

I can create a fantomas issue if you wish :)

@nojaf
Copy link
Contributor

nojaf commented Jan 13, 2025

I can create a fantomas issue if you wish :)

Only if it is a meaningful one, I'd say 😉. If your issue will be "hey we have a stack overflow somewhere in this codebase" then I wouldn't bother.

@KevinRansom KevinRansom added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Jan 13, 2025
@psfinaki
Copy link
Contributor

I'll merge this @progressive-galib. Thanks for addressing it!

@psfinaki psfinaki merged commit b2f2065 into dotnet:main Jan 14, 2025
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants