rok commented on pull request #11026:
URL: https://github.com/apache/arrow/pull/11026#issuecomment-911774550
Thanks for the fast feedback @ianmcook @lidavidm! :)
> Re the third parameter name: how about `first_week_is_fully_in_year` or
`first_week_fully_in_january`?
> The third parameter's naming is a little confusing - maybe
'first_week_is_full_week' or something?
`first_week_in_year` sounds most idiomatic to me. Changed.
> For the first parameter, would it be a lot of work to allow any day of the
week to be the start? It seems odd to have this be a boolean.
It wouldn't be much work no. My thinking here is: everyone starts the week
with Monday or Sunday let's not even allow for bad choices (we should probably
do the same for `DayOfWeekOption`). If we do know of calendars where week is
starts with some other day of the week then let's introduce an integer
parameter.
> The second parameter's doc is a little confusing, what is the behavior if
false?
It's now changed to say:
```
/// Dates from current year that fall into last ISO week of the previous
year return
/// 0 if true and 52 or 53 if false.
```
Does that make it clearer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
