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

time: add weekday helper functions #25469

Closed
vituchon opened this issue May 20, 2018 · 17 comments
Closed

time: add weekday helper functions #25469

vituchon opened this issue May 20, 2018 · 17 comments

Comments

@vituchon
Copy link

vituchon commented May 20, 2018

#Perhaps in future editions it would be nice to have a convenient function to get the following (or previous) week day of a given one.

Current Behaviour

As a client of time.Util library, for a given day if i rqquire to calculate the following one

today := time.Now()
tomorrow := today.Add(time.Duration(24) * time.Hour) // adds a day duration, could use (time.Time).AddDate  if really want to avoid the lead second
tomorrowWeekDay := tomorrow.WeekDay()

Motivation

the func (t Time) Weekday() Weekday does non-linear calculations (i do take a look at the code and see the logic behind and it is not trivial, and the order of the function could match a binary search "O(log n)" as the func (l *Location) lookup(sec int64) (name string, offset int, isDST bool, start, end int64) may be called during the process (look for l.lookup(sec) in time.Time source file) that could be simplified in the context of iterating through weekdays in a circular fashion (either forward or backward).

Possible Solution

// Gets the next week day, following golang time.WeekDay values convention
func next(day time.Weekday) time.Weekday {
	return time.Weekday((int(day) + 1) % 7)
}

The above was my implementation that works on my project.
As the language specs forbids: i can't use WeekDay as type receiver, thus, extending and binding the functionality to the concrete type; so i resort to write my own utility func.

Additional Test Code

func TestNextWeekDayWorks(t *testing.T) {
	// an entire loop over the week is enought to prove it works as it cover all posible values
	sunday := time.Sunday
	monday := getNextAndAssert(t, sunday, time.Monday)
	tuesday := getNextAndAssert(t, monday, time.Tuesday)
	wednesday := getNextAndAssert(t, tuesday, time.Wednesday)
	thursday := getNextAndAssert(t, wednesday, time.Thursday)
	friday := getNextAndAssert(t, thursday, time.Friday)
	saturday := getNextAndAssert(t, friday, time.Saturday)
	getNextAndAssert(t, saturday, time.Sunday)
}

func getNextAndAssert(t *testing.T, currentDay time.Weekday, expectedNextDay time.Weekday) (calculatedNextDay time.Weekday) {
	calculatedNextDay = next(currentDay)
	if calculatedNextDay != expectedNextDay {
		t.Errorf("Not equals.\nCalculated is: '%v', while expected is: '%v'", calculatedNextDay, expectedNextDay)
	}
	return
}

Context

My motivation arises for three reasons:

  1. by beign working with medical agendas that have plenty of relative time intervals (e.g: per day of week availability), and when the times comes to calculate the actual availabilties in a given lapse of time (that are consecutive days) i do find proper to work with time.WeekDay type in order to retrieve the relative availability information for the correspondent day.
  2. I would like to think that other persons already face or faced this scenario, i mean: to face the need of sequentially iterate over week days starting from a given absolute time.
  3. Need to be aware of golang convetion changes for days in a week concrete integer values.

I would love to receive some feedback about this including other professional's point of views.
Thanks and hope you find useful something of what i have just share.

@cznic
Copy link
Contributor

cznic commented May 20, 2018

The behavior I infer from the "Possible Solution" is simple modular arithmetic and it's directly supported by the language with no need for conversions: https://play.golang.org/p/v6XOucCpgUW.

Also, I don't see enough value in having the two special cases, prev and next, explicitly handled by extra functions/methods of the time package.

@kardianos kardianos changed the title Augment WeekDay with handy funcs to retrieve the immediate next and previous week days. time: add weekday helper functions May 20, 2018
@vituchon
Copy link
Author

cznic i see enought logic in your snippet of code... i leave up to you the decision to implement it as an function or not in the future releases of golang. It was only my humble suggestion and i think my words were very clear. i don't see why not, neither also you explain to me why not.

@vituchon vituchon reopened this May 21, 2018
@vituchon
Copy link
Author

vituchon commented May 21, 2018

func prevWeekday(d time.Weekday) time.Weekday { return (d - 1) % 7 }
func nextWeekday(d time.Weekday) time.Weekday { return (d + 1) % 7 }

and please, you want to every person (in every project) write these two functions over and over again?

@vituchon
Copy link
Author

vituchon commented May 21, 2018

is not my way to feel how "we must avoid to rebuild the whell", i insist to reflect on this cznic.

@vituchon
Copy link
Author

vituchon commented May 21, 2018

also reconsider the fact that we rely on abstractions in order build bigger system... so, nextWeekday and prevWeekday are part of those abstractions... after all, to implement a function like that in golang i have study before the golang convention for weekdays values (implying to be aware of any convention changes) and that take me time... so i only want to spare some time to others so they (as i) can spend it in more important things that are "above" this problem's domain. Just try to stay focus on our natural task: build abstrations, the right ones; and recall is not about complexity.

@cznic
Copy link
Contributor

cznic commented May 21, 2018

FTR

today := time.Now()
tomorrow := today.Add(time.Duration(24) * time.Hour) // adds a day duration
tomorrowWeekDay := tomorrow.WeekDay()

is not a correct solution of the problem.

@vituchon
Copy link
Author

i believe it works fine (and it does in prod) https://play.golang.org/p/lxbPySc8amm... i encourage you to explain why this is not a correct solution.

@cznic
Copy link
Contributor

cznic commented May 21, 2018

The duration between weekday changes is not fixed at 24 hours in the general case.

@bcmills
Copy link
Contributor

bcmills commented May 21, 2018

See also #19700.

@bcmills
Copy link
Contributor

bcmills commented May 21, 2018

As @cznic notes, you can already iterate over weekday constants using the usual mathematical operators, and the expression (d + 1) % 7 is not much boilerplate at all: you don't even need a function to abstract over it. For the general case, the combination of (time.Time).AddDate and (time.Time).Weekday also allow for iteration by arbitrary numbers of days.

For the specific problem of iterating over sequential days, this proposal doesn't seem to carry its weight: it would add API surface for trivial mathematical operations. For less-trivial use-cases, #19700 proposes a more comprehensive API. I'm closing this issue as a duplicate of that one.

@bcmills bcmills closed this as completed May 21, 2018
@vituchon
Copy link
Author

vituchon commented May 21, 2018

bcmills if i understand you right, you stated that it shall not be included because it is very simple logic. If that the case, let me point out clearly that we have diferent point of views here... i don't even put in trial the complexity of the algorithm, neither i care about how it is implemented. Please try to read "Context" section and please answer having in mind those points. I would appreciate very much that feedback, as a college or coworker point of view.

Thanks you both (also cznic) for the reading and the feedback.

@ianlancetaylor
Copy link
Contributor

New API always carries a cost. If we add a Next method to time.Weekday, we have to document it and maintain it. We also probably have to add a Prev method. The code is easy to write yourself, so as bcmills says this doesn't seem to carry its weight.

@vituchon
Copy link
Author

vituchon commented May 28, 2018

thanks for the reply ianlancetaylor. Allow me to highlight a previous question

¿do you want every person (in evey project) write these two pair of functions?

func prevWeekday(d time.Weekday) time.Weekday { return (d - 1) % 7 }
func nextWeekday(d time.Weekday) time.Weekday { return (d + 1) % 7 }

I mean, i know that is very light work to do, but as far i stated this could be a problem to more than one client of the library, so, the question i would make myself as client of the library is ¿why delegate the weigth to each client instead of putting it in a single place (like the "time" library) and once for all?.
I know is work to do and impacts on augmenting the complexity (just a bit, ok! is a bit and is annoying perhaps you may say) of the library; so that is why at first place my proposal was formally expressed among with a helpful (i hope) snippet of code. To recap: The "complexity" exists, and it shall be encapsulated in a layer. Is not an easy matter, it can has a lot of discussion, going and backs. As you can see, i'm participating on this, not only to help the comunnitty, also trying to obtain feedback that could be useful for myself and improve my way yo work with teams.

@cznic
Copy link
Contributor

cznic commented May 28, 2018

Encapsulation of complex expression and/or logic often makes sense. Encapsulating trivial expressions (two operators, single non-constant operand) not so much.

There is probably a lot if similar one-liners out there. Never will they all end in the stdlib, so someone must decide where to draw the line.

@vituchon
Copy link
Author

vituchon commented May 28, 2018

So we all recognize that the complexity exists and is fact (regardless the level of complexity).
If we put this in "client"/"server" terms: as a client you understand that i would strive for the "server" to implement it (and releases me from the burden that it takes), and i know that this is work for you by relieving me from do it. Allow me to add that i have already make the work, so you can imagine that this doens't release me from anything at this time, but i understand, and very weel, of the importance of this decision.

I see that neither any of my motivations (explained in the Motivations and Context Sections) is not enough to promote this "issue/request" to a "nice to have feature", and is OK. I do have interest of knowing what do you think of each of one of them, without any rush to answer, of course.

I do not know with how many work you have to deal with day by day, but for a start i will assume that is a lot. So I don't want to be annoying with this, and, if i'm about to be it, please excuse me. Thanks

@ianlancetaylor
Copy link
Contributor

I think I would summarize your motivations and contents as saying that these new methods would be useful. I agree. Nobody would propose ideas that aren't useful.

From my perspective, every change has both a benefit and a cost. We make changes where the benefit is larger than the cost. You've discussed the benefit. The various replies to you above discuss the cost.

In other words, we agree about the benefits you've mentioned. Where we disagree is whether the benefits are worth the cost.

@vituchon
Copy link
Author

vituchon commented May 28, 2018

And let me add that I believe that you guys are the ones that know the tradeoff of this, i mean, the cost and the benefits.

Thanks ian, for your feedback, it was very useful to me in order to understand better what is really the matter here, and therefore the thing that, perhaps, may be put on the table for discussion someday.

@golang golang locked and limited conversation to collaborators May 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants