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

Adding new ThreeWayComparable interface #457

Merged
merged 6 commits into from
Feb 28, 2023

Conversation

SamWheating
Copy link
Contributor

@SamWheating SamWheating commented Feb 27, 2023

Re: #356

This adds a new TotallyOrdered interface, similar to Comparable which returns an integer representing the <, ==, > relation between values. The CompareDepth function has been to updated check for this interface and handle this return style if applicable.

Also refactors int, float, time.Time and time.Duration types to use the new interface.

The previous Comparable interface will have to be maintained, not just for API compatibility but also for types which define equality/inequality but not ordering (for example, structs)

Open questions:

  • Is the ThreeWayCompareSameType name too verbose? I was thinking about overloading the name CompareSameType but that felt like it would get confusing. -> Yes, change to TotallyOrdered
  • Should I also update the other orderable types (float, str, list, etc) to use this interface in this PR? -> Yes.

Copy link
Collaborator

@adonovan adonovan left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution!

lib/time/time.go Show resolved Hide resolved
starlark/value.go Outdated Show resolved Hide resolved
starlark/value.go Outdated Show resolved Hide resolved
starlark/value.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@adonovan adonovan left a comment

Choose a reason for hiding this comment

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

A few more minor comments.

lib/time/time.go Show resolved Hide resolved
lib/time/time.go Outdated Show resolved Hide resolved
lib/time/time.go Outdated Show resolved Hide resolved
starlark/int.go Show resolved Hide resolved
Copy link
Collaborator

@adonovan adonovan 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. Many thanks again!

@adonovan adonovan merged commit dded032 into google:master Feb 28, 2023
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.

2 participants