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

Analyzer: Falsely matches HandleAsync method that isn't overriding #30

Closed
ardalis opened this issue Aug 7, 2020 · 6 comments
Closed

Analyzer: Falsely matches HandleAsync method that isn't overriding #30

ardalis opened this issue Aug 7, 2020 · 6 comments

Comments

@ardalis
Copy link
Owner

ardalis commented Aug 7, 2020

Handle methods that take querystring arguments will not 1:1 override base HandleAsync methods.

Example: the List method in the sample. It doesn't inherit from a generic base, so its base method doesn't define a Handle/HandleAsync to override.

image
@ardalis
Copy link
Owner Author

ardalis commented Aug 7, 2020

@ppittle do you want to look at this?

@fiseni
Copy link

fiseni commented Aug 7, 2020 •
edited
Loading

In the following section, we can add check for the actual name actually.
It works, but I'm not quite sure if you want to compare the name as string.

// isn't a new public method
if (methodSymbol.IsOverride || methodSymbol.DeclaredAccessibility != Accessibility.Public || methodSymbol.Name == "HandleAsync") 
    return;

@ppittle
Copy link
Contributor

ppittle commented Aug 8, 2020

Sure, should be able to TDD it

ppittle added a commit to ppittle/ApiEndpoints that referenced this issue Aug 8, 2020
@ppittle
Improve Code Analyzer heuristic and add additional tests for ardalis#30…
8aa8464 
…. Analyzer now ranks all public methods with the highest being the one that should be allowed
@ppittle ppittle mentioned this issue Aug 8, 2020
Merged
ardalis pushed a commit that referenced this issue Aug 9, 2020
@ppittle
Improve Code Analyzer (#31)
a9923e9 
* added test for analyzer flagging constructor as invallid and aligned style #29

* Improve Code Analyzer heuristic and add additional tests for #30.  Analyzer now ranks all public methods with the highest being the one that should be allowed
@ardalis
Copy link
Owner Author

ardalis commented Aug 10, 2020

After #31 I pushed v1.1.0 of the analyzers to Nuget, which I've now pulled into the sample.

That version still has this behavior on List (the other endpoints all have 0 warnings):

image

@ppittle
Copy link
Contributor

ppittle commented Aug 12, 2020

Reproduced in Studio. Interesting because that exact case should have been covered in the test ValidEndpointUsingNonGenericBaseClass.

Let me take a look.

ppittle added a commit to ppittle/ApiEndpoints that referenced this issue Aug 16, 2020
@ppittle
Improve Code Analyzer (ardalis#30). Ignore static methods and constru…
891168e 
…ctors.
ppittle added a commit to ppittle/ApiEndpoints that referenced this issue Aug 16, 2020
@ppittle
Improve Code Analyzer (ardalis#30). Ignore static methods and constru…
9ceca60 
…ctors.
@ppittle
Copy link
Contributor

ppittle commented Aug 16, 2020

@ardalis - still not 100% sure why the unit tests aren't able to pick up the bug where the Code Analyzer was not ignoring constructors.

I tested #33 manually and confirmed the issue was fixed, but could use a second pair of eyes to double check. 👀

@ppittle ppittle mentioned this issue Aug 16, 2020
Merged
ardalis pushed a commit that referenced this issue Aug 17, 2020
@ppittle
Improve Code Analyzer (#30). Ignore static methods and constructors. (#…
9255320 
…33)
@ardalis ardalis closed this as completed Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ardalis @ppittle @fiseni