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.

Sign up for GitHub

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

Jump to bottom

Create rule S6580: Use a format provider when parsing date and time #1737

Merged
merged 19 commits into from
Jul 5, 2023
Merged

Create rule S6580: Use a format provider when parsing date and time #1737

merged 19 commits into from
Jul 5, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Apr 11, 2023 •
edited by mary-georgiou-sonarsource
Loading

You can preview this rule here (updated a few minutes after each push).

Raise an issue for the Parse methods listed in the description if an IFormatProvider is not provided.

@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Create rule S6580 Create rule S6580: Always use DateTime.Parse overloads with a "CultureInfo" or parameter should be used Apr 11, 2023
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Create rule S6580: Always use DateTime.Parse overloads with a "CultureInfo" or parameter should be used Create rule S6580: Always use DateTime.Parse overloads with a "CultureInfo" or "IFormatProvider" parameter Apr 11, 2023
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Create rule S6580: Always use DateTime.Parse overloads with a "CultureInfo" or "IFormatProvider" parameter Create rule S6580: Always use DateTime.Parse overloads with an "IFormatProvider" parameter Apr 12, 2023
csaba-sagi-sonarsource
csaba-sagi-sonarsource suggested changes Apr 13, 2023
View reviewed changes
Copy link
Contributor

@csaba-sagi-sonarsource csaba-sagi-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few suggestions.

csaba-sagi-sonarsource
csaba-sagi-sonarsource suggested changes Apr 13, 2023
View reviewed changes
Copy link
Contributor

@csaba-sagi-sonarsource csaba-sagi-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second round

cristian-ambrosini-sonarsource
cristian-ambrosini-sonarsource suggested changes Apr 13, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few nitpicks, looks good!

rules/S6580/description.adoc Outdated Show resolved Hide resolved
rules/S6580/description.adoc Outdated Show resolved Hide resolved
rules/S6580/metadata.json Show resolved Hide resolved
rules/S6580/why.adoc Outdated Show resolved Hide resolved
cristian-ambrosini-sonarsource
cristian-ambrosini-sonarsource suggested changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there! I left a couple of comments

rules/S6580/description.adoc Outdated Show resolved Hide resolved
rules/S6580/description.adoc Outdated Show resolved Hide resolved
csaba-sagi-sonarsource
csaba-sagi-sonarsource reviewed Apr 14, 2023
View reviewed changes
rules/S6580/csharp/rule.adoc Outdated
[source,csharp,diff-id=1,diff-type=noncompliant]
----
var dateTimeString = "4/12/2023 4:05:48 PM"; // This is an en-US format string - 12 of April 2023
var dateTimeObject = DateTime.Parse(dateTimeString); // In a machine with "CultureInfo.CurrentCulture" en-150 (English Europe), this is parsed as 4th of December
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would somehow move this comment out from this line as it applies also for the other two lines where there is a runtime error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added part of this comment next to the Runtime error to be clearer why the runtime error happens.

In this line, I would leave this comment - as we don't have a runtime error but we parse one date and we get another one.
I made it more explicit though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please apply the same stuff to the vb.net code examples too.

csaba-sagi-sonarsource
csaba-sagi-sonarsource suggested changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

@csaba-sagi-sonarsource csaba-sagi-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last round

cristian-ambrosini-sonarsource
cristian-ambrosini-sonarsource approved these changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@sonarsource-next
Copy link

SonarQube Quality Gate for 'rspec-frontend'

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource changed the title Create rule S6580: Always use DateTime.Parse overloads with an "IFormatProvider" parameter Create rule S6580: Use a format provider when parsing date and time Jul 5, 2023
@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource marked this pull request as ready for review July 5, 2023 10:45
@sonarsource-next
Copy link

SonarQube Quality Gate for 'rspec-frontend'

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarsource-next
Copy link

SonarQube Quality Gate for 'rspec-tools'

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

csaba-sagi-sonarsource
csaba-sagi-sonarsource approved these changes Jul 5, 2023
View reviewed changes
Copy link
Contributor

@csaba-sagi-sonarsource csaba-sagi-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource merged commit a3e31a4 into master Jul 5, 2023
@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource deleted the rule/add-RSPEC-S6580 branch July 5, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mary-georgiou-sonarsource mary-georgiou-sonarsource mary-georgiou-sonarsource left review comments

@csaba-sagi-sonarsource csaba-sagi-sonarsource csaba-sagi-sonarsource approved these changes

@cristian-ambrosini-sonarsource cristian-ambrosini-sonarsource Awaiting requested review from cristian-ambrosini-sonarsource

Assignees

@mary-georgiou-sonarsource mary-georgiou-sonarsource

Labels
dotnet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@csaba-sagi-sonarsource @mary-georgiou-sonarsource @cristian-ambrosini-sonarsource @zsolt-kolbay-sonarsource