Skip to content

More improve completion after method/property override#18341

Merged
T-Gro merged 11 commits into
dotnet:mainfrom
ijklam:override-method-completion-improvement
Mar 17, 2025
Merged

More improve completion after method/property override#18341
T-Gro merged 11 commits into
dotnet:mainfrom
ijklam:override-method-completion-improvement

Conversation

@ijklam

@ijklam ijklam commented Feb 26, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

  1. Make indent in generated code depend on the context, not fix to 4.
  2. Add a switch to determine whether to generate an default implementation body for overridden method when completing. (See Improve completion after method/property override #17292 (comment))

图片

When checked:

GIF 2025-2-26 21-42-33

When unchecked:

GIF 2025-2-26 21-44-31

Checklist

  • Test cases added
  • Performance benchmarks added in case of performance changes
  • Release notes entry updated:

@ijklam ijklam requested a review from a team as a code owner February 26, 2025 13:46
@github-actions

github-actions Bot commented Feb 26, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

✅ No release notes required

@psfinaki

psfinaki commented Mar 5, 2025

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Mar 5, 2025

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

psfinaki
psfinaki reviewed Mar 5, 2025
View reviewed changes

@psfinaki psfinaki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, this is a nice feature!

Some minimal testing would be nice, otherwise - I think this is generally good to go.

Comment thread docs/release-notes/.FSharp.Compiler.Service/9.0.300.md Outdated
Comment thread docs/release-notes/.FSharp.Compiler.Service/9.0.300.md Outdated
Comment thread vsintegration/tests/FSharp.Editor.Tests/FsxCompletionProviderTests.fs
@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Mar 10, 2025
@T-Gro T-Gro enabled auto-merge (squash) March 10, 2025 16:13
auto-merge was automatically disabled March 13, 2025 12:45

Head branch was pushed to by a user without write access

@ijklam

ijklam commented Mar 13, 2025

Copy link
Copy Markdown
Contributor Author

How to fix this test? fsharp-ci (Build EndToEndBuildTests experimental_features)
fsharp-ci (Build EndToEndBuildTests experimental_features)

The log:

dotnet pack BasicProvider\BasicProvider.fsproj -o D:\a_work\1\s\tests\EndToEndBuildTests\BasicProvider\artifacts -c Release -v minimal -p:FSharpTestCompilerVersion=net40
Determining projects to restore...
D:\a_work\1\s\tests\EndToEndBuildTests\BasicProvider\BasicProvider\BasicProvider.fsproj : error MSB4057: The target "_GetRestoreSettingsPerFramework" does not exist in the project. [TargetFramework=net9.0]

@psfinaki

psfinaki commented Mar 13, 2025

Copy link
Copy Markdown
Contributor

Hi @ijklam it's now failing everywhere - not your fault. We're looking into that, sorry.

@Martin521 Martin521 mentioned this pull request Mar 13, 2025
Merged
@T-Gro T-Gro enabled auto-merge (squash) March 14, 2025 13:43
@T-Gro T-Gro merged commit f007405 into dotnet:main Mar 17, 2025
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 approved these changes

+1 more reviewer

@psfinaki psfinaki psfinaki approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

F# Compiler and Tooling
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ijklam @psfinaki @T-Gro