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

Forward CancellationToken analyzer and code fixer #5923

Merged
merged 23 commits into from May 21, 2021
Merged

Forward CancellationToken analyzer and code fixer #5923

merged 23 commits into from May 21, 2021

Conversation

DavidBoike
Copy link
Member

Replaces #5867

WIP until #5897 is merged

@DavidBoike DavidBoike mentioned this pull request Feb 15, 2021
Closed
@adamralph adamralph mentioned this pull request Feb 16, 2021
Closed
23 tasks
Base automatically changed from tokens to master February 18, 2021 18:33
@DavidBoike DavidBoike marked this pull request as ready for review February 18, 2021 21:18
@DavidBoike DavidBoike self-assigned this Feb 19, 2021
@adamralph adamralph force-pushed the forward-analyzer branch 4 times, most recently from 086df8c to 80dbd6e Compare March 2, 2021 09:37
@stale
Copy link

stale bot commented Apr 20, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 20, 2021
@adamralph adamralph removed the stale label Apr 20, 2021
@adamralph
Copy link
Member

It's alive, Jim.

adamralph
adamralph reviewed May 5, 2021
View reviewed changes
src/NServiceBus.Core.Analyzer/AwaitOrCaptureTasksAnalyzer.cs
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(diagnostic);

public override void Initialize(AnalysisContext context) => context.RegisterSyntaxNodeAction(Analyze, SyntaxKind.InvocationExpression);
public override void Initialize(AnalysisContext context) =>
context.WithDefaultSettings().RegisterSyntaxNodeAction(Analyze, SyntaxKind.InvocationExpression);
Copy link
Member

Choose a reason for hiding this comment

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

By calling WithDefaultSettings here, we are changing the behaviour of the existing analyzer, to ignore generated code. Is that desirable?

Copy link
Member Author

Choose a reason for hiding this comment

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

First thought: You don't fix generated code you fix the generator.

Second thought: It is a major.

I'm not sure we ever intended to analyze generated code.

Copy link
Member

Choose a reason for hiding this comment

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

But if generated code is ignored, how do you find out that the generator needs to be fixed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on this article and the high bar usages in the Roslyn Analyzers repo have documented for non-None use I'm going to leave it as is for now.

src/NServiceBus.Core.Analyzer/ForwardCancellationTokenAnalyzer.cs Outdated Show resolved Hide resolved
@adamralph
Copy link
Member

Regarding the build failure — #6051 (comment)

@adamralph adamralph mentioned this pull request May 6, 2021
Merged
@adamralph adamralph marked this pull request as draft May 6, 2021 14:03
@adamralph
Copy link
Member

Converting to draft until we've decided how to proceed here.

adamralph
adamralph reviewed May 21, 2021
View reviewed changes
Copy link
Member

@adamralph adamralph left a comment

Choose a reason for hiding this comment

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

From my POV, https://github.com/Particular/NServiceBus/pull/5923/files#r625785627 is the only outstanding thread here.

@adamralph adamralph marked this pull request as ready for review May 21, 2021 14:15
@DavidBoike DavidBoike merged commit 527f9c1 into master May 21, 2021
@DavidBoike DavidBoike deleted the forward-analyzer branch May 21, 2021 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamralph adamralph adamralph left review comments

@andreasohlund andreasohlund andreasohlund approved these changes

Assignees

@DavidBoike DavidBoike

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@DavidBoike @adamralph @andreasohlund