-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
#2765 Allow Filter instance reuse #2839
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
#2765 Allow Filter instance reuse #2839
Conversation
@martincostello I think this might solve this issue... This PR is not complete yet, but I am curious as to whether this approach is the way to go. If so I will add extension methods for all filter types |
Oh, this is about #2765 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a reasonable approach.
test/WebSites/MinimalAppWithConstructorDelay/MinimalAppWithConstructorDelay - Backup.csproj
Outdated
Show resolved
Hide resolved
test/WebSites/MinimalAppWithConstructorDelay/MinimalAppWithConstructorDelay.csproj
Outdated
Show resolved
Hide resolved
src/Swashbuckle.AspNetCore.SwaggerGen/DependencyInjection/SwaggerGenOptionsExtensions.cs
Show resolved
Hide resolved
src/Swashbuckle.AspNetCore.SwaggerGen/DependencyInjection/SwaggerGenOptionsExtensions.cs
Show resolved
Hide resolved
src/Swashbuckle.AspNetCore.SwaggerGen/DependencyInjection/ConfigureSwaggerGeneratorOptions.cs
Outdated
Show resolved
Hide resolved
src/Swashbuckle.AspNetCore.SwaggerGen/DependencyInjection/ConfigureSchemaGeneratorOptions.cs
Outdated
Show resolved
Hide resolved
@martincostello I just added that test website, to help investigate, I plan on deleting it. I just needed something to put the example program.cs in from the issue. This was mostly meant as a POC PR, to see if this would be the approach to solve it. I will now work on a proper solve. |
3c27f55
to
c43a0a1
Compare
test/Swashbuckle.AspNetCore.SwaggerGen.Test/ConfigureSwaggerGeneratorOptionsTests.cs
Show resolved
Hide resolved
src/Swashbuckle.AspNetCore.SwaggerGen/DependencyInjection/SwaggerGenOptionsExtensions.cs
Outdated
Show resolved
Hide resolved
src/Swashbuckle.AspNetCore.SwaggerGen/DependencyInjection/SwaggerGenOptionsExtensions.cs
Outdated
Show resolved
Hide resolved
src/Swashbuckle.AspNetCore.SwaggerGen/Properties/AssemblyInfo.cs
Outdated
Show resolved
Hide resolved
test/Swashbuckle.AspNetCore.SwaggerGen.Test/ConfigureSwaggerGeneratorOptionsTests.cs
Outdated
Show resolved
Hide resolved
test/Swashbuckle.AspNetCore.SwaggerGen.Test/Swashbuckle.AspNetCore.SwaggerGen.Test.snk
Outdated
Show resolved
Hide resolved
@martincostello I think I am done with this, pending any new comments |
The build is failing due to the strong naming changes. You might be hitting the complexity I noted here: #2805 (comment) If you can't easily resolve that, you'll have to create a manual mock and remove NSub. Also, can you apply the same exact changes as in this commit to your PR please? The CI is failing since GitHub updated the macOS runner: App-vNext/Polly@d7bad63 I could do it myself in a separate PR, but then it would hold things up waiting for someone else to review my changes. |
I configured all projects to be signed, hopefully that should fix the issue... locally the tests run fine now |
Signing any project we ship to NuGet that is not already strong-named is a massively breaking change for anyone using Swashbuckle via |
In fact, breaking or not, at this point this is far too much churn to allow the use of NSub in a few places for your test. Please revert all the signing changes and create a manual mock. |
Resolves #2765.