Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Do not duplicate directives when converting from property to methods #76184

Merged

Conversation

CyrusNajmabadi
Copy link
Member

Fixes #75186

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner November 30, 2024 20:22
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 30, 2024
@@ -1738,13 +1748,6 @@ internal interface ILanguageServiceHost
/// An value that provides access to the language service for the active configured project.
/// </returns>
object GetActiveProjectContext();

/// <summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

///

I can see an argument where the duplicated comments here provided value.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, i thought about it, but decided it was something the user would need to take care of to decide how they wanted the existing comment to map over to multiple members.

CancellationToken cancellationToken)
{
var methodDeclaration = GetSetMethodWorker();

// The analyzer doesn't report diagnostics when the trivia contains preprocessor directives, so it's safe
// to copy the complete leading trivia to both generated methods.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this comment used to be true? It seems like the code was written with consideration to pp directives, so I'm curious where it went wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah. that comment looks to be a big fat lie. it might have been true way in the past. :)

Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

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

:shipit:

@CyrusNajmabadi CyrusNajmabadi merged commit dd083dc into dotnet:main Dec 3, 2024
25 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the convertToMethodPPDirective branch December 3, 2024 00:51
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Dec 3, 2024
@dibarbet dibarbet modified the milestones: Next, 17.13 P3 Jan 7, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace 'xxx' with method has bug
3 participants