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

day of week cleanup #19212

Closed
wants to merge 2 commits into from
Closed

day of week cleanup #19212

wants to merge 2 commits into from

Conversation

simonbyrne
Copy link
Contributor

@simonbyrne simonbyrne commented Nov 3, 2016 •
edited
Loading

quinnj
quinnj reviewed Nov 4, 2016
View reviewed changes
test/dates/adjusters.jl
@@ -220,7 +220,7 @@ dt = Dates.Date(2014,5,21)
@test Dates.tonext(dt,Dates.Mon) == Dates.Date(2014,5,26)
@test Dates.tonext(dt,Dates.Tue) == Dates.Date(2014,5,27)
# No dayofweek function for out of range values
@test_throws KeyError Dates.tonext(dt,8)
#@test_throws KeyError Dates.tonext(dt,8)
Copy link
Member

Choose a reason for hiding this comment

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

Are you planning on adding this back in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's going to be removed (tonext no longer takes integer arguments).

@quinnj
Copy link
Member

quinnj commented Nov 4, 2016

Looking good.

@tkelman tkelman added the domain:dates Dates, times, and the Dates stdlib module label Nov 4, 2016
@simonbyrne simonbyrne mentioned this pull request Nov 4, 2016
Open
@simonbyrne simonbyrne changed the title WIP: day of week cleanup day of week cleanup Nov 4, 2016
femtotrader
femtotrader reviewed Nov 4, 2016
View reviewed changes
test/dates/adjusters.jl
@@ -219,8 +219,6 @@ dt = Dates.Date(2014,5,21)
@test Dates.tonext(dt,Dates.Sun) == Dates.Date(2014,5,25)
@test Dates.tonext(dt,Dates.Mon) == Dates.Date(2014,5,26)
@test Dates.tonext(dt,Dates.Tue) == Dates.Date(2014,5,27)
# No dayofweek function for out of range values
#@test_throws KeyError Dates.tonext(dt,8)

Copy link

Choose a reason for hiding this comment

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

A test with

@test Dates.firstdayofweek(Dates.DateTime(2013,12,24), firstday=Monday) == Dates.DateTime(2013,12,23)
@test Dates.firstdayofweek(Dates.DateTime(2013,12,24), firstday=Sunday) == Dates.DateTime(2013,12,22)

could be a good idea

and a similar test with lastdayofweek

Something like

@test Dates.lastdayofweek(Dates.DateTime(2013,12,24), firstday=Monday) == Dates.DateTime(2013,12,29)
@test Dates.lastdayofweek(Dates.DateTime(2013,12,24), firstday=Sunday) == Dates.DateTime(2013,12,28)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, done.

@simonbyrne simonbyrne force-pushed the sb/firstday branch 2 times, most recently from 2c3f69a to b2f3e8f Compare November 4, 2016 10:41
@simonbyrne
Copy link
Contributor Author

Okay, hopefully this should be ready. One question is whether the optional arguments to firstdayofweek/lastdayofweek should be keyword or positional? (I've gone with positional at the moment).

@nalimilan
Copy link
Member

dayname and dayabbr use a keyword argument locale, which is conceptually similar. So maybe use a keyword argument here too?

@simonbyrne
Copy link
Contributor Author

Also, should we deprecate ismonday, istuesday, etc? These seem like overkill.

@quinnj
Copy link
Member

quinnj commented Nov 4, 2016

I'd be fine with either positional or keyword + deprecating ismonday istuesday; those were only there for the Dates.tonext(dt, ::dayofweek) calls

@femtotrader femtotrader mentioned this pull request Nov 10, 2016
Open
@simonbyrne simonbyrne force-pushed the sb/firstday branch 3 times, most recently from a3c599c to 7de7c64 Compare November 17, 2016 21:11
@simonbyrne
* Make DayOfWeek an Enum, deprecate dayofweek. (#19210)
03bee90 
* Provide optional argument to `firsdayofweek`/`lastdayofweek` (#19208)
* A few misc fixes.
@simonbyrne
deprecate Dates.ismonday, etc
79a19f8
@simonbyrne
Copy link
Contributor Author

Okay, this is rebased and ready to go. Any objections?

@simonbyrne
Copy link
Contributor Author

@nanosoldier runbenchmarks("dates", vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@KristofferC
Copy link
Sponsor Member

That looks like an objection :p

@jrevels
Copy link
Member

jrevels commented Nov 18, 2016

I'd assume the regressions are from the deprecation right? If that's true, then the slowdown is expected and the benchmark should just be rewritten.

@simonbyrne
Copy link
Contributor Author

Well, the firstdayofweek is legitimately slower, as I changed the function definition slightly.

@tkelman
Copy link
Contributor

tkelman commented Jan 28, 2017

Do we still want to do this?

quinnj
quinnj requested changes Jan 28, 2017
View reviewed changes
Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

Yes, I'm still in favor of this. I don't think the previously noted performance regression should stop this; we're adding functionality that makes up for it.

base/dates/query.jl
"""
dayabbr(dt::Integer;locale::AbstractString="english") = VALUETODAYOFWEEKABBR[locale][dt]
DayOfWeek(dt::TimeType)
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd like to still keep this lowercase dayofweek, it follows the more general pattern in Dates with types being proper-cased and accessors being lowercased.

doc/stdlib/dates.rst
@@ -61,7 +61,7 @@ Dates Functions
---------------

All Dates functions are defined in the ``Dates`` module; note that only the ``Date``, ``DateTime``, and ``now`` functions are exported;
to use all other ``Dates`` functions, you'll need to prefix each function call with an explicit ``Dates.``, e.g. ``Dates.dayofweek(dt)``.
to use all other ``Dates`` functions, you'll need to prefix each function call with an explicit ``Dates.``, e.g. ``Dates.DayOfWeek(dt)``.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like these docs need to be moved from dates.rst.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Mar 28, 2022

Seems dead?

@quinnj quinnj closed this Apr 9, 2022
@giordano giordano deleted the sb/firstday branch May 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@femtotrader femtotrader femtotrader left review comments

@quinnj quinnj quinnj requested changes

Assignees
No one assigned
Labels
domain:dates Dates, times, and the Dates stdlib module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@simonbyrne @quinnj @nalimilan @nanosoldier @KristofferC @jrevels @tkelman @vtjnash @femtotrader