Skip to content

Conversation

@zverok
Copy link
Contributor

@zverok zverok commented Oct 19, 2022

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
@zverok
Copy link
Contributor Author

zverok commented Nov 13, 2022

@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 🤔

@zverok
Copy link
Contributor Author

zverok commented Nov 19, 2022

@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
Copy link
Contributor

@jeremyevans jeremyevans left a comment

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!

@zverok
Copy link
Contributor Author

zverok commented Nov 22, 2022

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
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