Skip to content

Tweak how we generate the IDisposable.Dispose pattern.#42426

Merged
CyrusNajmabadi merged 23 commits intodotnet:masterfrom
CyrusNajmabadi:disposeSimplify
Mar 17, 2020
Merged

Tweak how we generate the IDisposable.Dispose pattern.#42426
CyrusNajmabadi merged 23 commits intodotnet:masterfrom
CyrusNajmabadi:disposeSimplify

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

Fixes #9760

A few improvements:

  1. We no longer generate #regions. We stopped generating regions for interfaces years ago. This brings IDisposable in line with that.
  2. We keep fields grouped with fields as per common coding patterns.
  3. We respect user naming styles (i.e. underscores for field names).
  4. We follow slightly better patterns for the Dispose code we generate.

@CyrusNajmabadi CyrusNajmabadi requested a review from mavasani March 15, 2020 19:33
Comment thread src/EditorFeatures/CSharpTest/ImplementInterface/ImplementInterfaceTests.cs Outdated
Comment thread src/EditorFeatures/VisualBasicTest/ImplementInterface/ImplementInterfaceTests.vb Outdated
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

Ok. Have moved to a model where this is almost all done with standard code gen.

@CyrusNajmabadi CyrusNajmabadi requested a review from sharwell March 16, 2020 05:23
Module Program
Sub Main()
If True Then

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

changed because i tweaked the formatter to not introduce a blank line with nested if-statements + elastic trivia.

</data>
<data name="TODO_colon_uncomment_the_following_line_if_the_finalizer_is_overridden_above" xml:space="preserve">
<value>TODO: uncomment the following line if the finalizer is overridden above.</value>
</data>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

all resources moved to features layer.

return ifStatement.ReplaceToken(
ifStatement.GetLastToken(),
ifStatement.GetLastToken().WithPrependedLeadingTrivia(trivia));
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i couldn't find a language agnostic way to do this. So VB and C# both have way to add a comment inside their if-block.

return options.AutoInsertionLocation
? AddMembersToAppropiateLocationInDestination(destination, filteredMembers, availableIndices, options, cancellationToken)
: AddMembersToEndOfDestination(destination, filteredMembers, availableIndices, options, cancellationToken);
: AddMembersToEndOfDestination(destination, filteredMembers, options, cancellationToken);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

unused param.

INamespaceOrTypeSymbol destination,
Func<SyntaxNode, CodeGenerationOptions, IList<bool>, CancellationToken, SyntaxNode> declarationTransform,
CodeGenerationOptions options,
IEnumerable<ISymbol> members,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

unused param.

TypeOf previousStatement Is TypeStatementSyntax Then
TypeOf previousStatement Is TypeStatementSyntax OrElse
TypeOf previousStatement Is IfStatementSyntax Then
Return GetActualLines(previousToken, currentToken, 1)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

tweaked elastic formatting of nested ifs to not put in a blank line.

}}
#endregion";
// {string.Format(FeaturesResources.Do_not_change_this_code_Put_cleanup_code_in_0_method, "Dispose(bool disposing)")}
Dispose(disposing: true);
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.

💭 I'm not a huge fan of making this a named argument, but I'm ignoring it for now since the long-term goal is to remove the Dispose(bool) method altogether.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

Thanks!

@CyrusNajmabadi CyrusNajmabadi merged commit aad934c into dotnet:master Mar 17, 2020
@ghost ghost added this to the Next milestone Mar 17, 2020
@CyrusNajmabadi CyrusNajmabadi deleted the disposeSimplify branch March 17, 2020 00:05
@sharwell sharwell modified the milestones: Next, 16.6.P2 Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Our Dispose pattern is too noisy

4 participants