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

[Date] Implement %y year without century parser / formatter; respect digits that are below maximum format length #135

Merged
merged 8 commits into from
Jan 30, 2024

Conversation

ohaibbq
Copy link
Contributor

@ohaibbq ohaibbq commented Jan 26, 2024

Year numbers without leading zeros are allowed

Closes #134
Closes #137
Part of #138

@codecov-commenter
Copy link

codecov-commenter commented Jan 26, 2024

Codecov Report

Merging #135 (86afecf) into main (7b48c5e) will increase coverage by 0.42%.
The diff coverage is 88.59%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #135      +/-   ##
==========================================
+ Coverage   45.88%   46.31%   +0.42%     
==========================================
  Files          47       47              
  Lines       17822    17827       +5     
==========================================
+ Hits         8178     8256      +78     
+ Misses       8180     8126      -54     
+ Partials     1464     1445      -19     

@ohaibbq ohaibbq changed the title [Date] Implement %y year without century parser / formatter [Date] Implement %y year without century parser / formatter; respect digits that are below maximum format length Jan 26, 2024
Copy link
Owner

@goccy goccy left a comment

Choose a reason for hiding this comment

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

Thank you for your great PR ! I've commented.

if err != nil {
return 0, fmt.Errorf("unexpected month number")
return 0, 0, fmt.Errorf("%s", err)
Copy link
Owner

Choose a reason for hiding this comment

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

Please just return err

day := splitted[2]
if len(year) != 4 || len(month) != 2 || len(day) != 2 {
return 0, fmt.Errorf("unexpected year-month-day format")
if text[progress] != separator {
Copy link
Owner

Choose a reason for hiding this comment

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

Is a boundary check necessary ? ( if len(text) <= progress { return } )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A boundary check would be useful. I'll add it.

@ohaibbq ohaibbq requested a review from goccy January 30, 2024 02:35
@ohaibbq
Copy link
Contributor Author

ohaibbq commented Jan 30, 2024

@goccy Fixed and added tests! Thank you for your review 🎊

@ohaibbq ohaibbq requested a review from goccy January 30, 2024 03:08
y, err := strconv.ParseInt(year, 10, 64)
progress += 1

mProgress, m, err := parseDigitRespectingOptionalPlaces(text[progress:], 1, 12)
Copy link
Owner

Choose a reason for hiding this comment

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

Again, progress value is updated by L645, so boundary checks are necessary.

return 0, fmt.Errorf("could not parse hour:minute format: character after hour [%s] is not a [:]", string(text))
}
hProgress += 1
mProgress, m, err := parseDigitRespectingOptionalPlaces(text[hProgress:], 0, 59)
Copy link
Owner

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strings can be indexed at their ending boundary. The zero-length string will cause parseDigitRespectingOptionalPlaces to return an error.

	fmt.Println("123"[3:]) # empty string
	fmt.Println("123"[4:]) # index 4 out of bounds

Please see test case parse date with %F separator but no month for reproduction

Copy link
Owner

Choose a reason for hiding this comment

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

OK, It seems to be ok for slice processing.

@ohaibbq ohaibbq requested a review from goccy January 30, 2024 03:35
@goccy
Copy link
Owner

goccy commented Jan 30, 2024

LGTM 👍

@goccy goccy merged commit 961ce06 into goccy:main Jan 30, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants