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

C#: Japanese Era and Leap Year checks (Likely Bugs) #1345

Conversation

denislevin
Copy link
Contributor

CSharp checks for mishandling Japanese date and leap year calculations

@denislevin denislevin requested a review from a team as a code owner May 20, 2019 23:05
@denislevin denislevin changed the title Japanese Era and Leap Year checks (Likely Bugs) C#: Japanese Era and Leap Year checks (Likely Bugs) May 20, 2019
Copy link
Contributor

@calumgrant calumgrant left a comment

Choose a reason for hiding this comment

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

Many thanks for this. The query itself looks great, but I have added some style/typo comments.

csharp/ql/src/Likely Bugs/MishandlingJapaneseEra.cs Outdated Show resolved Hide resolved
csharp/ql/src/Likely Bugs/MishandlingJapaneseEra.cs Outdated Show resolved Hide resolved
csharp/ql/src/Likely Bugs/MishandlingJapaneseEra.cs Outdated Show resolved Hide resolved
csharp/ql/src/Likely Bugs/MishandlingJapaneseEra.cs Outdated Show resolved Hide resolved

from Expr expr, string message
where
isDateFromJapaneseCalendarToDateTime(expr) and message = "DateTime created from Japanese calendar with explicit or current era and hard-coded year" or
Copy link
Contributor

Choose a reason for hiding this comment

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

Single quotes around DateTime

Copy link
Contributor

Choose a reason for hiding this comment

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

Full stop at end of message.

from Expr expr, string message
where
isDateFromJapaneseCalendarToDateTime(expr) and message = "DateTime created from Japanese calendar with explicit or current era and hard-coded year" or
isDateFromJapaneseCalendarCreation(expr) and message = "DateTime constructed from Japanese calendar with explicit or current era and hard-coded year" or
Copy link
Contributor

Choose a reason for hiding this comment

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

Single quotes and full stop.

where
isDateFromJapaneseCalendarToDateTime(expr) and message = "DateTime created from Japanese calendar with explicit or current era and hard-coded year" or
isDateFromJapaneseCalendarCreation(expr) and message = "DateTime constructed from Japanese calendar with explicit or current era and hard-coded year" or
isEraCollectionCreation(expr) and message = "Hard-coded collection with Japanese era years" or
Copy link
Contributor

Choose a reason for hiding this comment

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

Full stop.

isDateFromJapaneseCalendarToDateTime(expr) and message = "DateTime created from Japanese calendar with explicit or current era and hard-coded year" or
isDateFromJapaneseCalendarCreation(expr) and message = "DateTime constructed from Japanese calendar with explicit or current era and hard-coded year" or
isEraCollectionCreation(expr) and message = "Hard-coded collection with Japanese era years" or
inEraArrayCreation (expr) and message = "Hard-coded array with Japanese era years" or
Copy link
Contributor

Choose a reason for hiding this comment

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

Full stop.

isDateFromJapaneseCalendarCreation(expr) and message = "DateTime constructed from Japanese calendar with explicit or current era and hard-coded year" or
isEraCollectionCreation(expr) and message = "Hard-coded collection with Japanese era years" or
inEraArrayCreation (expr) and message = "Hard-coded array with Japanese era years" or
isExactEraStartDateCreation(expr) and message = "Hard-coded the beginning of the Japanese Heisei era"
Copy link
Contributor

Choose a reason for hiding this comment

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

Full stop.

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments. I just have one more small request.

@@ -0,0 +1,42 @@
using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this file, and others, from csharp/ql/test/query-tests/Likely Bugs/LeapYear/UnsafeYearConstruction/ to csharp/ql/test/query-tests/Likely Bugs/UnsafeYearConstruction/ (i.e., remove the nested LeapYear folder).

Copy link
Contributor

@calumgrant calumgrant left a comment

Choose a reason for hiding this comment

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

A few small things then it's good to go. Many thanks!

@calumgrant calumgrant merged commit a3d5d2c into github:master Jun 20, 2019
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 this pull request may close these issues.

4 participants