#2765 Allow Filter instance reuse

[remcolam]

Resolves #2765.

[remcolam]

@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

[remcolam]

Oh, this is about #2765

[martincostello]

This seems a reasonable approach.

[remcolam]

@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.

[remcolam]

@martincostello I think I am done with this, pending any new comments

[martincostello]

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.

[remcolam]

I configured all projects to be signed, hopefully that should fix the issue... locally the tests run fine now

[martincostello]

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 netstandard2.0 in a .NET Framework application. We cannot make such a change.

[martincostello]

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.

[remcolam]

I do not think NSub is actually causing this, it is because I am trying to give access to an internal method in my test

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.16%. Comparing base (3416927) to head (a92e0ff).

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2839      +/-   ##
==========================================
+ Coverage   92.06%   92.16%   +0.10%     
==========================================
  Files          93       93              
  Lines        3111     3152      +41     
  Branches      536      553      +17     
==========================================
+ Hits         2864     2905      +41     
  Misses        247      247              

Flag	Coverage Δ
Linux	92.16% <100.00%> (+0.10%)	⬆️
Windows	?
macOS	92.16% <100.00%> (+0.10%)	⬆️

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.

[martincostello]

In that case keeping NSub is fine, but giving internals access is too complicated due to the inconsistent use of strong-naming if it requires changing anything beyond test projects (which is why I noted the complication in #2805 after trying to do the same thing myself in another PR).

Either you'll need to make the member public (but only if that makes sense to, not just because it makes testing convenient), or test it through other means at an integration/end-to-end level.

[remcolam]

@martincostello I only needed to also sign the TestSupport Project for it to work. All checks have passed, I think this should be it

[martincostello]

Why did you revert the AssemblyInfo change when it works?

[remcolam]

The tests were failing on it, because your suggestion had the Key misspelled.. Took me too long to figure out why weird stuff was happening... but should be good now

[martincostello]

Thanks again!

[remcolam]

@martincostello you are not a fan of:
x = x ?? throw new ArgumentNullException()

But what do you think of:
x ??= throw new ArgumentNullException()

That shouldn't do an assignment, and only throw the exception.

Problem is the language spec... If netstandard needs to be supported your out of luck, I believe

[martincostello]

It's a bit better, but still not great IMHO as that's just syntactic sugar for the first I think (if x is null then assign...oh wait, throw).

If API availability for different TFMs wasn't a problem, I would just do this:

ArgumentNullException.ThrowIfNull(x);

Otherwise I'd just do it "old school":

if (x == null)
{
    throw new ArgumentNullException(nameof(x));
}

[remcolam]

@martincostello I like these kind code style of discussions.

I think if (x == null) throw new ArgumentnullExcpetion() is also ok, since it is on one line. However, I really do not like if statements without scope braces.

This I hate:

if (something)
   DoThis()
else
  DoSomethingeElse() 

This just invites unintended codeflow, when you want to add anywhere: AndAlsoDoThis()

[remcolam]

If the whole if is on a single line, I can accept no braces, but once you add linebreaks, add braces