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

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

Merged
merged 14 commits into from
Jan 14, 2025
Merged

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

merged 14 commits into from
Jan 14, 2025

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
@progressive-galib
/Compiler/TypedTree/TypedTreePickle.fs refactored.
3fddcbc 
"#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
Merge branch "dotnet:main" into 18061-DependencyProvider
877b7f1
@progressive-galib progressive-galib requested a review from a team as a code owner January 7, 2025 08:14
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 7, 2025 •
edited
Loading

⚠️ 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 Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 8, 2025

Failed to run fantomas: https://github.com/dotnet/fsharp/actions/runs/12667153205

@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
Member

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 •
edited
Loading

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
Member

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 •
edited
Loading

@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
Member

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

@psfinaki
Copy link
Member

@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
Member

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
Reviewers

@T-Gro T-Gro T-Gro left review comments

@psfinaki psfinaki psfinaki approved these changes

@KevinRansom KevinRansom KevinRansom approved these changes

Assignees
No one assigned
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
F# Compiler and Tooling
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@progressive-galib @T-Gro @Martin521 @psfinaki @nojaf @KevinRansom