Remove CA1305 suppressions (part3 final) #4754

martinrrm · 2022-08-11T17:07:46Z

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/1170

Regression? Last working version:

Description

Removing suppressions for CA1305, I'm also changing the rule action to be Warning but this is causing some error on ReadOnly files, I added file suppressions for these ones since I cannot modify them.

PR Checklist

dominoFire

Some comments to start.

src/NuGet.Core/NuGet.CommandLine.XPlat/GlobalSuppressions.cs

test/TestUtilities/Test.Utility/ProtocolUtility.cs

martinrrm · 2022-08-12T17:41:46Z

@dominoFire With your comment, I looked again at the changes and updated some of the suppressions to InvariantCulture when it is about URLs

src/NuGet.Core/NuGet.CommandLine.XPlat/GlobalSuppressions.cs

src/NuGet.Core/NuGet.Commands/GlobalSuppressions.cs

nkolev92 · 2022-08-15T18:16:02Z

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/IDE/VSSolutionManager.cs

            _solutionEvents.ProjectRenamed += OnEnvDTEProjectRenamed;

-            var vSStd97CmdIDGUID = VSConstants.GUID_VSStandardCommandSet97.ToString("B", CultureInfo.CurrentCulture);
+            var vSStd97CmdIDGUID = VSConstants.GUID_VSStandardCommandSet97.ToString("B", CultureInfo.InvariantCulture);


I left a comment in a previous PR after it had been merged (mb), I think we should ignore the warnings raised for Guid ToString as this has the potential to break eeventually.

I thought you mean to change it to InvariantCulture but I can add the suppressions for this. Or I can pass null to the method call. https://docs.microsoft.com/en-us/dotnet/api/system.guid.tostring?view=net-6.0#system-guid-tostring(system-string-system-iformatprovider)

null's fine if we mean to avoid the warning

dominoFire

More partial review, 60% review left.

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/TableParser.cs

src/NuGet.Core/NuGet.Commands/GlobalSuppressions.cs

src/NuGet.Core/NuGet.PackageManagement/Telemetry/ActionEventBase.cs

src/NuGet.Core/NuGet.Packaging/PackageReaderBase.cs

nkolev92

That's a lot of changes :D

Good job getting all tedious work done.

dominoFire

Good job! Some comments on cases where it could be better to use InvariantCulture

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/IDE/VSSolutionManager.cs

src/NuGet.Core/NuGet.Protocol/Resources/RawSearchResourceV3.cs

dominoFire · 2022-08-30T19:55:24Z

test/TestUtilities/Test.Utility/TestPackages.cs

            for (int i = 0; i < 10000; i++)
            {
-                sb.AppendLine(i.ToString());
+                sb.AppendLine(i.ToString(CultureInfo.CurrentCulture));


You can append the integer directly

Suggested change

sb.AppendLine(i.ToString(CultureInfo.CurrentCulture));

sb.AppendLine(i);

Unfortunately, AppendLine only accepts a string

src/NuGet.Core/NuGet.Packaging/Signing/DerEncoding/DerSequenceReader.cs

src/NuGet.Core/NuGet.Protocol/LegacyFeed/V2FeedQueryBuilder.cs

src/NuGet.Core/NuGet.Protocol/Plugins/Logging/PluginLogMessage.cs

src/NuGet.Core/NuGet.Protocol/Resources/AutoCompleteResourceV3.cs

heng-liu · 2022-08-30T23:58:06Z

src/NuGet.Core/NuGet.Packaging/Signing/Timestamp/Timestamp.cs

            if (flags == SignatureVerificationStatusFlags.NoErrors)
            {
-                issues.Add(SignatureLog.InformationLog(string.Format(CultureInfo.CurrentCulture, Strings.TimestampValue, GeneralizedTime.LocalDateTime.ToString()) + Environment.NewLine));
+                issues.Add(SignatureLog.InformationLog(string.Format(CultureInfo.CurrentCulture, Strings.TimestampValue, GeneralizedTime.LocalDateTime.ToString(CultureInfo.CurrentCulture)) + Environment.NewLine));


There seems no behavior change. I think this is expected?
string.Format(CultureInfo.CurrentCulture, s1, dateTime.ToString()) is equal to
string.Format(CultureInfo.CurrentCulture, s1, dateTime.ToString(CultureInfo.CurrentCulture)), right?

ghost · 2022-09-07T01:02:08Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

martinrrm requested a review from a team as a code owner August 11, 2022 17:07

dominoFire reviewed Aug 11, 2022

View reviewed changes

src/NuGet.Core/NuGet.CommandLine.XPlat/GlobalSuppressions.cs Outdated Show resolved Hide resolved

test/TestUtilities/Test.Utility/ProtocolUtility.cs Outdated Show resolved Hide resolved

martinrrm requested a review from dominoFire August 12, 2022 17:40

erdembayar reviewed Aug 12, 2022

View reviewed changes

src/NuGet.Core/NuGet.CommandLine.XPlat/GlobalSuppressions.cs Show resolved Hide resolved

erdembayar reviewed Aug 12, 2022

View reviewed changes

src/NuGet.Core/NuGet.Commands/GlobalSuppressions.cs Outdated Show resolved Hide resolved

nkolev92 requested changes Aug 15, 2022

View reviewed changes

dominoFire reviewed Aug 16, 2022

View reviewed changes

martinrrm requested review from dominoFire, erdembayar and nkolev92 August 24, 2022 18:15

nkolev92 previously approved these changes Aug 25, 2022

View reviewed changes

martinrrm dismissed nkolev92’s stale review via cefb350 August 30, 2022 17:01

martinrrm force-pushed the dev-martinrrm-remove-CA1305-suppresions-part3 branch from 14aaa04 to cefb350 Compare August 30, 2022 17:01

dominoFire reviewed Aug 30, 2022

View reviewed changes

heng-liu reviewed Aug 30, 2022

View reviewed changes

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 7, 2022

ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 7, 2022

martinrrm requested a review from dominoFire September 7, 2022 18:08

martinrrm added 9 commits September 7, 2022 11:08

remove suppresions for CA1305

08b99d8

continuation

48af212

fix CI

24ffcef

fix CI build

a9d6475

pr comments

025e48a

change rule action

5c8f5c5

pr comments

66aa52d

changing guids to null when format needed

90c2176

pr comments

39e83fc

martinrrm force-pushed the dev-martinrrm-remove-CA1305-suppresions-part3 branch from ea7320d to 39e83fc Compare September 7, 2022 18:08

dominoFire approved these changes Sep 7, 2022

View reviewed changes

nkolev92 approved these changes Sep 7, 2022

View reviewed changes

martinrrm merged commit ee9ef1f into dev Sep 7, 2022

martinrrm deleted the dev-martinrrm-remove-CA1305-suppresions-part3 branch September 7, 2022 21:58

	sb.AppendLine(i.ToString(CultureInfo.CurrentCulture));
	sb.AppendLine(i);

Remove CA1305 suppressions (part3 final) #4754

Remove CA1305 suppressions (part3 final) #4754

Uh oh!

Conversation

martinrrm commented Aug 11, 2022

Bug

Description

PR Checklist

Uh oh!

dominoFire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

martinrrm commented Aug 12, 2022

Uh oh!

Uh oh!

Uh oh!

nkolev92 Aug 15, 2022

Choose a reason for hiding this comment

Uh oh!

martinrrm Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkolev92 Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

dominoFire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkolev92 left a comment

Choose a reason for hiding this comment

Uh oh!

dominoFire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dominoFire Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

martinrrm Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

heng-liu Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

martinrrm Aug 16, 2022 •

edited

Loading