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

Deprecate negate keyword argument in Dates adjuster API #20213

Merged
merged 3 commits into from
Jan 25, 2017
Merged

Conversation

tkelman
Copy link
Contributor

@tkelman tkelman commented Jan 24, 2017

This seems very unnecessary, especially now that we have unary ! for functions. Does anyone use this? We could probably remove the DateFunction wrapper as well but that's less user-visible so shouldn't matter as much when it happens.

Unfortunately keyword arguments are pretty messy to deprecate at the moment.

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

@tkelman tkelman added the domain:dates Dates, times, and the Dates stdlib module label Jan 24, 2017
@tkelman tkelman requested review from omus and quinnj January 24, 2017 12:05
@tkelman tkelman added the kind:deprecation This change introduces or involves a deprecation label Jan 24, 2017
Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

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

Nice work. I was unaware that you could negate functions via func = !func.

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.

Looks great! Yes, the composition of ! is a great new feature!

@ararslan
Copy link
Member

Looks like Nanosoldier didn't get triggered?

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

@jrevels
Copy link
Member

jrevels commented Jan 24, 2017

Network error, had to kick the Nanosoldier:

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

@tkelman
Copy link
Contributor Author

tkelman commented Jan 24, 2017

yeah, #17155 hasn't had a news entry added yet

@nanosoldier
Copy link
Collaborator

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

@tkelman
Copy link
Contributor Author

tkelman commented Jan 25, 2017

I took the fact that this needed a rebase after #19920 as an opportunity to push a slight refactoring commit, which should hopefully be exactly equivalent, aside from being more concise.

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

@nanosoldier
Copy link
Collaborator

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

@tkelman tkelman merged commit 0751779 into master Jan 25, 2017
@tkelman tkelman deleted the tk/negatenegate branch January 25, 2017 20:00
@Sacha0 Sacha0 added the needs news A NEWS entry is required for this change label May 13, 2017
@tkelman tkelman removed the needs news A NEWS entry is required for this change label May 14, 2017
tkelman pushed a commit that referenced this pull request May 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dates Dates, times, and the Dates stdlib module kind:deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants