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

Add deconstruct_keys for Date and DateTime and Time #1023

Merged
merged 1 commit into from
May 11, 2023
Merged

Add deconstruct_keys for Date and DateTime and Time #1023

merged 1 commit into from
May 11, 2023

Conversation

AI-Mozi
Copy link
Contributor

@AI-Mozi AI-Mozi commented May 10, 2023 •
edited by andrykonchin
Loading

#1016
Issue

Time#deconstruct_keys is added, allowing to use Time instances
in pattern-matching expressions [Feature #19071]

@AI-Mozi AI-Mozi force-pushed the deconstruct_keys_specs_for_date branch from 5419469 to 8217f19 Compare May 11, 2023 06:45
@AI-Mozi AI-Mozi changed the title Add deconstruct_keys for Date and DateTime Add deconstruct_keys for Date and DateTime and Time May 11, 2023
@AI-Mozi AI-Mozi force-pushed the deconstruct_keys_specs_for_date branch 3 times, most recently from b812ebc to 7054dae Compare May 11, 2023 07:05
@AI-Mozi AI-Mozi marked this pull request as ready for review May 11, 2023 07:17
@AI-Mozi
Copy link
Contributor Author

AI-Mozi commented May 11, 2023

Should I also add some examples into language/pattern_matching_spec.rb ?

andrykonchin
andrykonchin reviewed May 11, 2023
View reviewed changes
Copy link
Member

@andrykonchin andrykonchin 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 to me, but there is one issue to address.

library/date/deconstruct_keys_spec.rb Show resolved Hide resolved
library/datetime/deconstruct_keys_spec.rb Show resolved Hide resolved
library/time/deconstruct_keys_spec.rb Outdated Show resolved Hide resolved
@andrykonchin
Copy link
Member

Should I also add some examples into language/pattern_matching_spec.rb ?

What test cases would you add there? I haven't found any test case for any Core Library class (Hash, Struct) there.

@AI-Mozi AI-Mozi force-pushed the deconstruct_keys_specs_for_date branch from 7054dae to ad78070 Compare May 11, 2023 12:33
@AI-Mozi
Copy link
Contributor Author

AI-Mozi commented May 11, 2023

I didn't pay much attention to the specs in pattern_matching_spec.rb. Actually there is no need for Time specs there :)

andrykonchin
andrykonchin approved these changes May 11, 2023
View reviewed changes
Copy link
Member

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

core/time/deconstruct_keys_spec.rb Show resolved Hide resolved
@AI-Mozi AI-Mozi force-pushed the deconstruct_keys_specs_for_date branch from ad78070 to 38c178c Compare May 11, 2023 16:40
@AI-Mozi
add deconstruct_keys for Date and DateTime
8719340 
add spec for `Time`

add more specs, move `Time` specs to core
@AI-Mozi AI-Mozi force-pushed the deconstruct_keys_specs_for_date branch from 38c178c to 8719340 Compare May 11, 2023 16:41
@andrykonchin
Copy link
Member

Thank you! It looks pretty good.

@andrykonchin andrykonchin merged commit 624c7aa into ruby:master May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrykonchin andrykonchin andrykonchin approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AI-Mozi @andrykonchin