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

Make TimeAmount Hashable #1571

Merged
merged 1 commit into from
Jun 24, 2020
Merged

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Jun 24, 2020

Motivation:

TimeAmount wasn't Hashable and there's no clear reason it shouldn't
be.

Modifications:

Add synthesized Hashable conformance to TimeAmount

Result:

TimeAmount is hashable.

@glbrntt glbrntt requested a review from Lukasa June 24, 2020 10:00
@glbrntt glbrntt added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Jun 24, 2020
@glbrntt glbrntt added this to the 2.19.0 milestone Jun 24, 2020
@glbrntt glbrntt requested a review from weissi June 24, 2020 10:01
@glbrntt
Copy link
Contributor Author

glbrntt commented Jun 24, 2020

(NIODeadline is already Hashable)

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Fab. While the API compatibility checker should catch this, want to just add a quick test to validate behaviour? Easiest test is to create three TimeAmount values, two the same and one different, then throw them in a Set and confirm it contains what you expect.

Motivation:

TimeAmount wasn't `Hashable` and there's no clear reason it shouldn't
be.

Modifications:

Add synthesized `Hashable` conformance to `TimeAmount`

Result:

`TimeAmount` is hashable.
@glbrntt
Copy link
Contributor Author

glbrntt commented Jun 24, 2020

5.0 failure is #1147

@glbrntt
Copy link
Contributor Author

glbrntt commented Jun 24, 2020

@swift-nio-bot test this please

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

awesome, thanks

@weissi weissi merged commit 8a25c0f into apple:master Jun 24, 2020
@glbrntt glbrntt deleted the gb-timeamount-hashable branch June 24, 2020 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants