Fix logic around out-of-order arguments for classic assertions (#712)

[Bartleby2718]

I found another bug while fixing the code, but in the interest of making the PR more targeted and reviewable, I'll file another issue and another PR to fix that.

Closes #712.

[Bartleby2718]

@dotnet-policy-service agree

[manfred-brands]

@Bartleby2718 You are right that most code fixes don't take named parameters into account and I rather not have 24 separate updates for each of the classic code fixes.

The code already makes assumptions before getting to the UpdateArguments method you updated:

            arguments[0] = CastIfNecessary(arguments[0]);
            if (arguments.Count > 1)
                arguments[1] = CastIfNecessary(arguments[1]);

Can I suggest to do this differently?
Instead of the UpdateArguments expecting a List<ArgumentSyntax> we could change it to a Dictionary<string, ArgumentSyntax>. That way the code fixes can change arguments[0] with arguments[NUnitFrameworkConstants.NameOfExpectedParameter].

Actually the whole UpdateArgument should be changed to return a new argument list or just an '(actual, constraint)' tuple as all codefixes do updates like arguments.RemoveAt(0) and arguments.Insert(2, ...) which are all wrong when arguments are specified out of order.

The method could then do something like: return (actualArgument, constraintArgument) and let the calling method deal with the message argument.

[manfred-brands]

Once named arguments are in play, none of the code using an index is valid.
See comment above to change approach to replace underlying design.

[Bartleby2718]

@manfred-brands I think there's a little more room for improvement, but I've pushed what I did today. Given the time difference, requesting a re-review now.

[manfred-brands]

@Bartleby2718 Thanks for making the changed.
The actual codefix now uses parameter names iso indices is a good approach.

I put some remarks for changes in the PR.

[Bartleby2718]

I'll need some refactoring to make sure the code follows the conventions of this repository, is maintainable, and is readable.

In terms of correctness, however, I think the code is alright. Hence, re-requesting review from @manfred-brands!

[manfred-brands]

@Bartleby2718 I think you are on the correct path.
Change how to treat the message parameter form List to Dictionary.
Some cleanups.

And then doing the other Classic Code Fixes ...

[manfred-brands]

However, I'm not sure how many tests should be added to other *CodeFixTests files, although I'm sure we need some.

The exisiting tests should ensure that nothing gets broken. Then I suggest one new test per Codefix using named out of order parameters.

[Bartleby2718]

@dotnet-policy-service agree

[Bartleby2718]

Still working on the tests; there's a lot to add 😅

[manfred-brands]

Nice work!
A few changes to make "more standard" named parameters work.

I'm not convinced we should introduce named parameters for the constraint model, regardless if they were used in the classic model.
I rather drop all names (.WithNameColon(null)) than replace arg1 with actual.

[manfred-brands]

@Bartleby2718 I pushed up a commit addressing some of what I commented on in the PR.

[Bartleby2718]

@manfred-brands Could you assign the PR to me?

[manfred-brands]

I noticed that not all 'named parameters' where removed in the classic code fixes.
I added a commit to consistently use .WithNameColon(null) when obtaining parameters and updated the tests.

[manfred-brands]

Thanks for the cooperation.
I think we are there and the actual code fixes have become more straight forward.

[Bartleby2718]

@manfred-brands Thank you for all the guidance and explanations! I feel like I've learned a lot about both NUnit and Roslyn analyzers.

I have a couple more questions about this repo:

1. If I were to create another PR, should it target master? I wonder how you prevent merge conflicts if you don't merge PRs immediately.
2. When will the next version of NUnit.Analyzers be released? Will it be the last week of May to match the release schedule of NUnit (unless there is a patch released before that)?

[manfred-brands]

I have a couple more questions about this repo:

1. If I were to create another PR, should it target master? I wonder how you prevent merge conflicts if you don't merge PRs immediately.

Normally a PR targets master. If PRs are for different areas there should normally not be a merge conflict.
If a PR build upon a previous one it can be on top of an other branch.

PRs are merged when approved. In this case I want to see if @mikkelbu has any comments before merging.

2. When will the next version of NUnit.Analyzers be released? Will it be the last week of May to match the release schedule of NUnit (unless there is a patch released before that)?

I didn't know there was a NUnit release schedule. I thought they are released when enough changes justify one. NUnit and the NUnit.Analyzers don't have to be in sync and are released independently at the moment.

[mikkelbu]

I've only reviewed half the files, but so far it looks great. I won't have time to review the rest of the files before the end of the week as I've very little spare time currently - hopefully Thursday evening CET or perhaps Friday evening CET.

I've left a couple of comments, but nothing major.

And Manfred is correct about the release cadency of NUnit vs NUnit.Analyzers. We could release the Analyzers when this PR is done and all the current active PRs have been merged.

[manfred-brands]

@mikkelbu Fixed your reported issues and found two items we forgot to clean up. I left them in as separate commits.

[mikkelbu]

Looks great @Bartleby2718 and @manfred-brands. I only noticed one little nit-pick otherwise I think we should merge.

I especially like how it simplified several of the existing analyzers/fixes.

[Bartleby2718]

@mikkelbu @manfred-brands Addressed the last feedback. Thanks both for the feedback and answers and guidance!