Skip to content

[Ruby 3.0 support] Add category kwarg to Kernel.warn and Warning.warn #2533

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

Merged
merged 4 commits into from
Nov 18, 2021

Conversation

Strech
Copy link

@Strech Strech commented Nov 14, 2021

Also, some existing behaviour were ported to other Warning methods, like type check in Warning#[] and Warnign#[]=

In addition, some ruby specs were added ruby/spec#897

Implementation concerns

  1. I'm a bit concerned about that line. In MRI implementation C-code shares that functionality between warn and [], both call the same function which returns internal category value, but not sure that I can reuse it as I did
  2. Not sure that I fully understand that line does it mean that I have to copy some checks like inclusion in a pre-defined list of values?

@Strech Strech force-pushed the add-category-to-warning-warn branch from 4e3c69f to 170a288 Compare November 14, 2021 21:41
@Strech Strech changed the title Add category kwarg to Kernel.warn and Warning.warn [Ruby 3.0 support] Add category kwarg to Kernel.warn and Warning.warn Nov 14, 2021
@eregon
Copy link
Member

eregon commented Nov 15, 2021

  • I'm a bit concerned about that line. In MRI implementation C-code shares that functionality between warn and [], both call the same function which returns internal category value, but not sure that I can reuse it as I did

Calling [] in itself is fine, but it's not ideal to read the value needlessly.
I think it's better to have a method checking if the category is correct.

Could you add a method doing that, under module Truffle::WarningOperations which can define in the same file?

  • Not sure that I fully understand that line does it mean that I have to copy some checks like inclusion in a pre-defined list of values?

I can't find what this link points to, can you post the comment in the diff instead?

@Strech
Copy link
Author

Strech commented Nov 15, 2021

Calling [] in itself is fine, but it's not ideal to read the value needlessly.
I think it's better to have a method checking if the category is correct.

Ok, have exactly the same thoughts about unnecessary reads, will adjust the code

I can't find what this link points to, can you post the comment in the diff instead?

It's because it's hidden, oh, here is a file link

if Primitive.object_equal(self, Warning) # avoid recursion when redefining Warning#warn
unless message.encoding.ascii_compatible?
raise Encoding::CompatibilityError, "ASCII incompatible encoding: #{message.encoding}"
end
$stderr.write message

@eregon
Copy link
Member

eregon commented Nov 15, 2021

That's a weird corner case when people incorrectly redefine Warning#warn to super into Kernel#warn.
I think it doesn't matter too much, but yeah let's do the category check there too for consistency.

* Add Symbol check to Warning#[] and Warnign#[]=
@Strech Strech force-pushed the add-category-to-warning-warn branch from 170a288 to 85ce09c Compare November 15, 2021 17:31
@Strech
Copy link
Author

Strech commented Nov 15, 2021

@eregon I've done with all the suggestions (except writing test, since there is one already)

@Strech Strech requested a review from eregon November 16, 2021 20:26
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! (and sorry for the late review)

@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Nov 18, 2021
@eregon eregon self-assigned this Nov 18, 2021
@eregon eregon added this to the 22.0.0 milestone Nov 18, 2021
@eregon eregon added the shopify Pull requests from Shopify label Nov 18, 2021
@eregon eregon mentioned this pull request Nov 18, 2021
82 tasks
@graalvmbot graalvmbot merged commit f439b38 into oracle:master Nov 18, 2021
@Strech
Copy link
Author

Strech commented Nov 19, 2021

@eregon Oh, finally get what you've meant by old warn support, thanks for adjustments!

@Strech Strech deleted the add-category-to-warning-warn branch November 19, 2021 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. oca-signed shopify Pull requests from Shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants