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.

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

Support Results #3096

Conversation

jgarciadelanoceda
Copy link
Contributor

Fixes #2595

@codecov-commenter
Copy link

codecov-commenter commented Oct 3, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 97.43590% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.50%. Comparing base (c051ca6) to head (fa4dfa0).

Files with missing lines Patch % Lines
...re.SwaggerGen/SwaggerGenerator/SwaggerGenerator.cs 97.43% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3096      +/-   ##
==========================================
- Coverage   90.50%   88.50%   -2.00%     
==========================================
  Files          76       97      +21     
  Lines        3116     3542     +426     
  Branches      515      628     +113     
==========================================
+ Hits         2820     3135     +315     
- Misses        296      407     +111     
Flag Coverage Δ
Linux 90.54% <97.43%> (+0.04%) ⬆️
Windows 88.43% <97.36%> (-2.07%) ⬇️
macOS 90.54% <97.43%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Javier García de la Noceda Argüelles added 2 commits October 3, 2024 14:02
@jgarciadelanoceda
Copy link
Contributor Author

It turns out to be nonPublic the method

@martincostello
Copy link
Collaborator

It turns out to be nonPublic the method

Feels wrong - if it's a method on an interface that's public it should be public, and if it's genuinely non public we shouldn't depend on it.

@jgarciadelanoceda
Copy link
Contributor Author

jgarciadelanoceda commented Oct 3, 2024

It turns out to be nonPublic the method

Feels wrong - if it's a method on an interface that's public it should be public, and if it's genuinely non public we shouldn't depend on it.

Yeah.. That's why I created the PR dotnet/aspnetcore#57464, because at first I did not like either the reflection there involved.. But it's not approved yet, and if it was approved I do not think it'll get backported to dotnet8 or yet dotnet9.. Because it's not a critical Bug this one

@jgarciadelanoceda
Copy link
Contributor Author

I mark this as closed, it's a good addition, but there is risk involved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please support Results<T> as return type of controllers
3 participants