Skip to content

Add Time#deconstruct_keys - #6594

Merged
zverok merged 1 commit into
ruby:masterfrom
zverok:time-deconstruct
Nov 22, 2022
Merged

Add Time#deconstruct_keys#6594
zverok merged 1 commit into
ruby:masterfrom
zverok:time-deconstruct

Conversation

@zverok

@zverok zverok commented Oct 19, 2022

Copy link
Copy Markdown
Contributor

I believe that Time being suitable for pattern-matching is a reasonable feature with many possible usages, which will increase usability of Time and would be a good show case for pattern matching.

Implementation decisions

UPD: As per Matz's decision, only #deconstruct_keys is implemented now.

Time#deconstruct_keys:

  • chosen keys: [:year, :month, :day, :yday, :wday, :hour, :min, :sec, :subsec, :dst, :zone]
  • I am open to discussing whether we should include other subsecond units (or any whatsoever)
  • It might be useful (but too loose interface) to support mon as a synonym for month?.. But might be confusing if somebody will unpack the **rest
  • day, not mday, seems most reasonable

Possible usages:

case t
in year: ...2022
  puts "too old"
in month: ..9
  puts "quarter 1-3"
in wday: 1..5, month:
  puts "working day in month #{month}"
end

if t in Time(wday: 3, day: ..7)
  puts "first Wednesday of the month"
end

Time#to_h: added on a "why not" basis :) As we already have "convert to hash" in the form of deconstruct_keys(nil), having a canonic form seems harmless. Open for discussion.
Time#deconstruct: returns time components in order [year, month, mday, hour, min, sec, subsec]

@zverok zverok changed the title Add Time#deconstruct, #deconstruct_keys, and #to_h Add Time#deconstruct_keys Oct 22, 2022
@zverok
zverok requested review from jeremyevans and nobu November 7, 2022 08:52
Comment thread time.c
Comment thread time.c Outdated
Comment thread time.c Outdated
Comment thread time.c
Comment thread time.c Outdated
Comment thread time.c Outdated
@zverok

zverok commented Nov 13, 2022

Copy link
Copy Markdown
Contributor Author

@jeremyevans You were completely right, macros were overthinking, removed them, and changed IDs to symbols for new keys, it looks cleaner this way.

I still need advice on updating dependencies, tool/update-deps didn't change anything 🤔

Comment thread time.c Outdated
Comment thread time.c
@zverok

zverok commented Nov 19, 2022

Copy link
Copy Markdown
Contributor Author

@jeremyevans Yeah, sorry, you are completely right. Trying to "quickly wrap it up" after exhausting week was not a good call of mine 🤷

@zverok
zverok requested a review from jeremyevans November 19, 2022 16:53

@jeremyevans jeremyevans left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, thanks for working on this!

Comment thread time.c Outdated
@zverok

zverok commented Nov 22, 2022

Copy link
Copy Markdown
Contributor Author

I updated NEWS and fixed the typo, should I wait for @nobu's review, or can it be merged already? 🐱
(I have merge rights, just not sure yet of the proper process, the months since I received the rights were ... turbulent)

@jeremyevans

Copy link
Copy Markdown
Contributor

Should be fine to merge after CI completes.

@zverok
zverok merged commit eaf2b6c into ruby:master Nov 22, 2022
@zverok
zverok deleted the time-deconstruct branch November 22, 2022 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants