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

Add more types for Random (fix conflict #470) #1144

Merged
merged 6 commits into from
Nov 24, 2022
Merged

Conversation

unasuke
Copy link
Contributor

@unasuke unasuke commented Nov 13, 2022

original pull request is #470

This is my first time writing RBS so I am not sure it is right, especially the tests.

I am not sure how to fully describe rand because it can return whatever type you give it, for example:

Random.rand(Tally.new("|")..Tally.new("|||||"))
=> #<Tally:0x00007fedf725f9f8 @string="|">

In other words, rand: (Range[SomeType]) -> SomeType but I don't know how to write that in RBS.

(Also, I think SomeType might have to be a descendent of Numeric, but the documentation is not clear)

What I do

  • Fix types by comments on original pull request
  • Merge master into this branch and resolve conflicts

Why I do

I need changesets of the #470 (especially Random.urandom) but original author did not respond mainteiner's review comments from 2 years.

core/random.rbs Outdated
@@ -71,6 +91,7 @@ class Random < RBS::Unnamed::Random_Base
def self.rand: () -> Float
| (Integer | ::Range[Integer] max) -> Integer
| (Float | ::Range[Float] max) -> Float
| (::Range[T]) -> T
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the type we want would be [T <: Numeric] (Range[T]) -> T, but RBS currently doesn't allow writing it.

I think there are two options. [T] (Range[T]) -> T or (Range[Numeric]) -> Numeric. The first option would make sense in my opinion, because testing the code will uncover the problem anyway.
#470 (comment)

Is [T] (Range[T]) invalid syntax?

Copy link
Member

Choose a reason for hiding this comment

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

[T] (Range[T]) is not a valid piece of code. It becomes a valid method type by adding the returned type, like [T] (Range[T]) -> T as soutaro said.

By the way, [T < Numeric] (Range[T]) -> T is more appropriate because the upper bound type for a type parameter has been implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pocke Thanks. Fixed at 224a180

@soutaro soutaro added this to the RBS 2.8.0 milestone Nov 24, 2022
Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

@soutaro soutaro merged commit f62363b into ruby:master Nov 24, 2022
@unasuke unasuke deleted the random branch November 24, 2022 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants