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 with_*** helpers #1687

Merged
merged 4 commits into from
Dec 19, 2023
Merged

Add with_*** helpers #1687

merged 4 commits into from
Dec 19, 2023

Conversation

soutaro
Copy link
Member

@soutaro soutaro commented Dec 19, 2023

This PR moves the helpers like with_int from test/stdlib to lib/rbs/unit_test, so that other gems can use them to test their type definitions.

Comment on lines 64 to 71
def with_array(*elements)
return WithEnum.new to_enum(__method__ || raise, *elements) unless block_given?

yield elements
yield ToArray.new(*elements)
end

def with_hash(hash = {})
Copy link
Contributor

Choose a reason for hiding this comment

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

with_array(*elements), but not with_hash(**elements)?

Note: kwargs isn’t Symbols only and that – good?

p('any key' => 'is valid', 420 => 69)
  #=> {"any key"=>"is valid", 420=>69}

Copy link
Member Author

Choose a reason for hiding this comment

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

We can pass hash without curly brace with the current definition. So any other good reason to make it kwargs?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just prefer consistency.

Comment on lines 115 to 130
BlankSlate = RBS::UnitTest::Convertibles::BlankSlate
ToIO = RBS::UnitTest::Convertibles::ToIO
ToI = RBS::UnitTest::Convertibles::ToI
ToInt = RBS::UnitTest::Convertibles::ToInt
ToF = RBS::UnitTest::Convertibles::ToF
ToR = RBS::UnitTest::Convertibles::ToR
ToC = RBS::UnitTest::Convertibles::ToC
ToStr = RBS::UnitTest::Convertibles::ToStr
ToS = RBS::UnitTest::Convertibles::ToS
ToSym = RBS::UnitTest::Convertibles::ToSym
ToA = RBS::UnitTest::Convertibles::ToA
ToArray = RBS::UnitTest::Convertibles::ToArray
ToHash = RBS::UnitTest::Convertibles::ToHash
ToPath = RBS::UnitTest::Convertibles::ToPath
CustomRange = RBS::UnitTest::Convertibles::CustomRange
Each = RBS::UnitTest::Convertibles::Each
Copy link
Contributor

Choose a reason for hiding this comment

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

include RBS::UnitTest::Convertibles?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah this is in fact for RBS runtime type checker. I'm not sure if including works for it...

@ksss
Copy link
Collaborator

ksss commented Dec 19, 2023

I see potential for property based testing (or fuzzing) in this framework.
Do you have any plans for such an extension in the future?

with_int(count: 100) do |i|
  # Yields 100 times random integer value
end

module Convertibles
class BlankSlate < BasicObject
instance_methods.each do |im|
next if %i[__send__ __id__].include? im
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to remove __id__ from this list, as it doesnt warn when you undef it

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. Will try it.

def each(&block) = @enum.each(&block)

def and_nil(&block)
self.and(nil, &_ = block)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add return WithEnum.new to_enum(__method__ || raise) unless block here

Copy link
Member Author

Choose a reason for hiding this comment

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

and accepts no block call. So it should be ok.


# An object with `#to_io` method
class ToIO < BlankSlate
@io: untyped
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these untypeds be given types? ditto for initialize and to_xxx for each of the ToXXX classes

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the type of IO here? I think it's not very clear and decided to keep them untyped.

Copy link
Contributor

Choose a reason for hiding this comment

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

@io should be ::IO; Same for ToStr and String, etc


# Yields `::array` objects
#
def with_array: (*untyped elements) { (array[untyped]) -> void } -> void
Copy link
Contributor

Choose a reason for hiding this comment

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

We should give this a type, like def with_array: [T] (*T elements). Same with hash and range

Copy link
Member Author

Choose a reason for hiding this comment

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

Skip range method because it requires <=> method.

Copy link
Contributor

Choose a reason for hiding this comment

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

unintentional inclusion?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's unrelated, but I think it's acceptable to mix it.

@soutaro soutaro added this pull request to the merge queue Dec 19, 2023
Merged via the queue into master with commit 2fdb2e9 Dec 19, 2023
23 checks passed
@soutaro soutaro deleted the with_helpers branch December 19, 2023 08:05
soutaro added a commit that referenced this pull request Dec 20, 2023
This reverts commit 2fdb2e9, reversing
changes made to b2689e7.
@soutaro soutaro added the Released PRs already included in the released version label Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Released PRs already included in the released version
Development

Successfully merging this pull request may close these issues.

4 participants