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

Improve variable type name request #957

Merged
merged 3 commits into from
May 4, 2023
Merged

Conversation

adisonlampert
Copy link
Contributor

Description

It is possible to override the name method in a class with a method that has arguments, resulting in an ArgumentError when calling klass.name. #790 addresses this by handing the exception and setting the variable's type to "<Error: #{e.message} (#{e.backtrace.first}>".

This PR improves the handling of the case where the class method is overwritten and removes the need for the type_name method by using M_NAME.bind_call(klass) instead of calling the name method. Instead of the variable's type being the error message, it is now more accurate:

Before

{
          "name": "f",
          "value": "#<Foo:0x00000001090377a8>",
          "type": "<Error: wrong number of arguments (given 0, expected 1) (/var/folders/lf/pp4810hd7n5d0c_zs0skjxfh0000gn/T/debug-20230329-68605-bb7wh3.rb:5:in `to_s'>",
          "variablesReference": 4,
          "indexedVariables": 0,
          "namedVariables": 1
}

After

{
          "name": "f",
          "value": "#<Foo:0x00000001090377a8>",
          "type": "Foo",
          "variablesReference": 4,
          "indexedVariables": 0,
          "namedVariables": 1
}

(This example is from the test included in this PR)


begin
klass.name || klass.to_s
rescue Exception => e
Copy link
Collaborator

@ko1 ko1 Apr 4, 2023

Choose a reason for hiding this comment

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

why does it drop klass.to_s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe type_name is already a String so I don't think to_s is necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No. klass.name can return nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, I see. Thanks for the catch!
I updated the code to use to_s and added a test for anonymous class instances to ensure the type is "".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you really need nil for the name?
Previous one is klass.name || klass.to_s but your code uses klass.name.to_s.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The previous code means that if klass.name is given, use it, otherwise use klass.to_s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the code so that the type is set to klass.to_s if there is no type name and I updated the related test (test_anonymous_class_instance).

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, the error because of to_s is ignored.
I'll fix it after merging.

@adisonlampert adisonlampert force-pushed the al/add-test-for-790 branch 2 times, most recently from d59e3ca to a136d09 Compare April 5, 2023 21:02
@ko1 ko1 merged commit 7b1ef07 into ruby:master May 4, 2023
ko1 pushed a commit that referenced this pull request May 4, 2023
* use `M_NAME` to call `Module#name`
* use parens to avoid warnings: `warning: ambiguity between regexp and two divisions...`
ko1 pushed a commit that referenced this pull request May 4, 2023
* use `M_NAME` to call `Module#name`
* use parens to avoid warnings: `warning: ambiguity between regexp and two divisions...`
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.

2 participants