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

Warn use of params for assertion messages #562

Closed
OsirisTerje opened this issue Jul 10, 2023 · 9 comments · Fixed by #612
Closed

Warn use of params for assertion messages #562

OsirisTerje opened this issue Jul 10, 2023 · 9 comments · Fixed by #612
Assignees

Comments

@OsirisTerje
Copy link
Member

OsirisTerje commented Jul 10, 2023

Ref issue nunit/nunit#4413 and Issue nunit/nunit#3936 - introduction of CallerArgumentExpressions (CAE) and nunit/nunit#4415.

Since we need to have the CAE at the end of the parameter list, we can't have the params there. The use of params for messages are now superseded by the use of string interpolation.

The analyzer should warn about this usage and suggest conversion to an interpolated string.

@manfred-brands
Copy link
Member

@mikkelbu I have created an analyzer for this, but it must be run when still compiling against NUnit3 as the code will not compile against NUnit4. My question is do we want to release this as 3.9.0 or as part of 4.0.0?
We could make 3.9.0 a prepare for NUnit4 release.
If releasing it as 4.0.0-beta, people might upgrade both the analyzers and nunit at the same time.

@mikkelbu
Copy link
Member

Alternatively, I should release version 3.9 with the existing fixes now, so that we get a more clean "prepare"/"beta" release (that is more focused on changes relating to NUnit4 ?

@manfred-brands
Copy link
Member

How would we convey that users need to upgrade nunit.analyzers to 4 before unit?

The changes in the nunit4 branch work for both nunit3 and nunit4. Where needed, the analyzer detects what version of nunit is referenced.

@mikkelbu
Copy link
Member

How would we convey that users need to upgrade nunit.analyzers to 4 before unit?

I think the best way is to do a proper release (or more) of the analyzers before NUnit 4 is released. I plan to release version 3.9 as it is now (but I'll first have time by the end of this week). Then we can use the next release as a "prepare for NUnit 4 release".
I'm not sure if this release should be called 3.10.0 or 4.0.0. I'm leaning towards the latter as I think this will fit best with the scope of the changes. It can be that 3.10.0 will make more people upgrade immediately as the "step" from 3.9 to 3.10 sounds smaller than 3.9 to 4.0, but I guess most will just upgrade to the latest version and see if it works.

We could also do some advertising for the analyzers on the NUnit homepage and the NUnit twitter account (I'm actually not sure who controls this account, but I guess it is Rob or Terje), but I'm not sure how many follows these sources. Alternatively, we could also see if we could do an announcement via the dotnet foundation.

@OsirisTerje
Copy link
Member Author

OsirisTerje commented Oct 25, 2023

It should also be clearly stated in the package description. That is what is most close to the user.
But I guess we need to get this information in all possible places. Anyway, NUnit 4 IS a breaking change version.

@rprouse controls the twitter account.

we could do an announcement via the dotnet foundation

I think this is a great suggestion. We haven't sent much to the monthly newsletter there for ages, so that would be great!

@rprouse
Copy link
Member

rprouse commented Oct 25, 2023

@OsirisTerje I can send you the credentials for the Twitter account and/or post for you.

@OsirisTerje
Copy link
Member Author

@rprouse Thanks, please send them to me - makes it easier to do announcements, but given the nature of Twitter right now, please hold on to them too. Europe and Twitter seems to not be so good friends these days.

@MaceWindu
Copy link

MaceWindu commented Nov 14, 2023

Are #612 fixes detect and warn about use of CallerArgumentExpression arguments by user?

With 3.9 following code compiles without any warnings in 4.0:

Assert.That(something, Is.True, "{0} {1}", stringValue, anotherStringValue);

@manfred-brands
Copy link
Member

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 a pull request may close this issue.

5 participants