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

Kernel instance methods #1945

Merged

Conversation

sampersand
Copy link
Contributor

@sampersand sampersand commented Aug 2, 2024

This PR updates all the instance methods on Kernel, and also adds tests for them.

core/kernel.rbs Show resolved Hide resolved
core/kernel.rbs Show resolved Hide resolved
@@ -2782,8 +2779,7 @@ module Kernel : BasicObject
#
# See #respond_to?, and the example of BasicObject.
#
%a{annotate:rdoc:copy:Object#respond_to_missing?}
private def respond_to_missing?: (Symbol, bool) -> bool
private def respond_to_missing?: (Symbol | String name, bool include_all) -> bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default implementation doesn't actually check the arguments and always returns false, but the documentation says it can also take a String, so I added that.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is meant to be an abstract method but with a default implementation given.

core/kernel.rbs Outdated Show resolved Hide resolved
@sampersand sampersand marked this pull request as ready for review August 2, 2024 05:11
@sampersand sampersand force-pushed the swesterman/24-08-01/Kernel_instance_methods branch 8 times, most recently from 0a3ac8c to b1a6b95 Compare August 4, 2024 03:29
@@ -2447,7 +2445,7 @@ module Kernel : BasicObject
# b.instance_of? B #=> true
# b.instance_of? C #=> false
#
def instance_of?: (Module) -> bool
def instance_of?: (Module | Class module_or_class) -> bool
Copy link
Member

Choose a reason for hiding this comment

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

Technically Class < Module, and the | Class is redundant.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While the | Class is technically redundant, i think it works well as a documentation—new ruby users might not know that all classes are modules, and might be confused as to why is_a? and friends don't say they accept Classes

Copy link
Contributor

Choose a reason for hiding this comment

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

Has there been discussion in Ruby Core on making Class not outright inherit from Module?

Class is not a strict subtype of Module because of the many places that can complain “wrong argument type Class (expected Module) (TypeError)”.

include Class


%a{annotate:rdoc:skip}
# <!--
Copy link
Member

Choose a reason for hiding this comment

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

Wow, we have two RDoc entries for yield_self and then... 🤔

@sampersand sampersand force-pushed the swesterman/24-08-01/Kernel_instance_methods branch from db0988c to 81d239d Compare August 5, 2024 04:49
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.

👍

@soutaro soutaro added this to the RBS 3.6 milestone Aug 9, 2024
@soutaro soutaro added this pull request to the merge queue Aug 9, 2024
Merged via the queue into ruby:master with commit 26cf5f6 Aug 9, 2024
19 checks passed
@soutaro soutaro added the Released PRs already included in the released version label Sep 17, 2024
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.

3 participants