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.

Sign up for GitHub

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

Jump to bottom

Bug/kernel#warn uplevel #2442

Merged

3 commits merged into rubygems:master from nobu:bug/Kernel#warn-uplevel

Oct 25, 2018

Contributor

nobu commented Oct 22, 2018

Description:

With this script w.rb:

require "Win32API"

The expected line is shown when rubygems is disabled.

$ ruby -w --disable=gems -I./lib -I./ext/win32/lib w.rb 
w.rb:1: warning: Win32API is deprecated after Ruby 1.9.1; use fiddle directly instead

But kernel_require.rb is shown when rubygems is enabled.

$ ruby -w --enable=gems -I./lib -I./ext/win32/lib w.rb 
/opt/local/lib/ruby/2.6.0/rubygems/core_ext/kernel_require.rb:54: warning: Win32API is deprecated after Ruby 1.9.1; use fiddle directly instead

Tasks:

Describe the problem / feature
Write tests
Write code to solve the problem
Get code review from coworkers / friends

I will abide by the code of conduct.

bronzdoc added the status: triage label

nobu mentioned this pull request

Support Keyword args with Pysch. #2439

Merged

4 tasks

nobu force-pushed the bug/Kernel#warn-uplevel branch from 7b1b772 to 74a092f Compare

October 23, 2018 14:39

segiddins reviewed

View reviewed changes

.travis.yml Outdated

@@ @@ -8,6 +8,7 @@ branches: @@
                   - trying
                   - /^[\d.]+$/
                   - /.+-stable$/
+                  - /^bug\//

Member

segiddins Oct 24, 2018

adding this shouldn't be necessary, since you're making a PR into a branch that's already tested

Contributor

MSP-Greg Oct 24, 2018

Fork. Fork. Fork. Nothing is nicer than working on a PR in one's fork and discovering that you can't run a test on it because the branch name isn't allowed.

Also, I think it's also used in another popular repo, one that Nobu and some others here occasionally contribute to...

Contributor Author

nobu Oct 25, 2018

Yes, the last commit is unnecessary, and I'll remove it.
It is just to run CIs before creating this PR.

appveyor.yml Outdated

@@ @@ -4,6 +4,7 @@ branches: @@
                 - master
                 - auto
                 - /[\d.]+/
+                - /^bug/

Member

segiddins Oct 24, 2018

same as above

duckinator requested changes

View reviewed changes

Member

duckinator left a comment

Hi, thanks for the PR!

If you could revert 7506506 (which made unnecessary CI changes) and address the comments I made, that'd be fantastic. 🙂

Let me know if you need clarifications on any of my comments, or if you need any help!

lib/rubygems/core_ext/kernel_warn.rb Show resolved Hide resolved

lib/rubygems/core_ext/kernel_warn.rb Show resolved Hide resolved

lib/rubygems/core_ext/kernel_warn.rb Outdated

+                  module_function define_method(:warn) {|*messages, uplevel: nil|
+                    if uplevel
+                      uplevel, = [uplevel].pack("l!").unpack("l!")

Member

duckinator Oct 24, 2018

What is this even doing?

Contributor Author

nobu Oct 25, 2018

Converting uplevel to an Integer which fits long, with calling to_int if needed.
There is no such method right now.

lib/rubygems/core_ext/kernel_warn.rb Outdated

+                        start = 0
+                        begin
+                          loc, = caller_locations(start, 1)
+                          break start += uplevel unless loc

Member

duckinator Oct 24, 2018

This is a bit more readable:

unless loc
  start += uplevel
  break start
end

lib/rubygems/core_ext/kernel_warn.rb Outdated

+                          loc, = caller_locations(start, 1)
+                          break start += uplevel unless loc
+                          start += 1
+                        end while (loc.path.start_with?(path) or (uplevel -= 1) >= 0)

Member

duckinator Oct 24, 2018

Two things with how this loop is done:

loop do/end and a conditional break is generally preferable over begin/end while ....
Please avoid assignments in conditions, especially complicated conditions like this one.

lib/rubygems/core_ext/kernel_warn.rb Show resolved Hide resolved

lib/rubygems/core_ext/kernel_warn.rb Show resolved Hide resolved

nobu added 2 commits

October 25, 2018 21:37


          Test for uplevel option of warn

284e340


          kernel_warn.rb: skip kernel_require.rb

7153a31

* lib/rubygems/core_ext/kernel_warn.rb (Kernel#warn): skip
  kernel_require.rb's frames when `uplevel` option is given.

nobu force-pushed the bug/Kernel#warn-uplevel branch from bcc81f8 to edfe2ac Compare

October 25, 2018 12:37


          kernel_warn.rb: adjust styles

98b351d

* added comments
* prefer `while` over `begin`/`end while`
* prefer guard clauase over `if`..`else`
* avoid assignments in conditions

nobu force-pushed the bug/Kernel#warn-uplevel branch from edfe2ac to 98b351d Compare

October 25, 2018 12:59

duckinator approved these changes

View reviewed changes

Member

duckinator commented Oct 25, 2018

Thanks for the PR, @nobu!

ghost pushed a commit that referenced this pull request


          Merge #2442

cc7f7dc

2442: Bug/kernel#warn uplevel r=duckinator a=nobu

# Description:

With this script `w.rb`:
```ruby
require "Win32API"
```

The expected line is shown when rubygems is disabled.
```
$ ruby -w --disable=gems -I./lib -I./ext/win32/lib w.rb 
w.rb:1: warning: Win32API is deprecated after Ruby 1.9.1; use fiddle directly instead
```

But `kernel_require.rb` is shown when rubygems is enabled.
```
$ ruby -w --enable=gems -I./lib -I./ext/win32/lib w.rb 
/opt/local/lib/ruby/2.6.0/rubygems/core_ext/kernel_require.rb:54: warning: Win32API is deprecated after Ruby 1.9.1; use fiddle directly instead
```

______________

# Tasks:

- [x] Describe the problem / feature
- [x] Write tests
- [x] Write code to solve the problem
- [ ] Get code review from coworkers / friends

I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).

Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org>

Member

duckinator commented Oct 25, 2018

@nobu by the way, thank you for taking the time to explain the parts of this PR I asked about. The .pack('l!').unpack('l!') and your approach to suppressing that warning are things I hadn't encountered previously, and I wanted to at least understand what they did before merging it.

ghost commented Oct 25, 2018

Build succeeded

continuous-integration/travis-ci/push

ghost merged commit 98b351d into rubygems:master

bronzdoc removed the status: triage label

nobu deleted the bug/Kernel#warn-uplevel branch

October 26, 2018 05:06

Contributor

MSP-Greg commented Dec 19, 2018

I believe this PR/commit includes the file rubygems/core_ext/kernel_warn.rb, so the only way you could have a load error is if you're using a new version of rubygems.rb but an old version of the files in rubygems/core_ext?

hsbt mentioned this pull request

gem 3.0.0 `require': cannot load such file #2535

Closed

5 tasks

deivid-rodriguez mentioned this pull request

Fix Kernel#warn override #2911

Merged

4 tasks

This was referenced Oct 1, 2020

avoiding Kernel.warn patch #3053

Closed

Fix "stack level too deep" error when overriding Warning.warn #3987

Merged

eregon mentioned this pull request

Do not override Kernel#warn when there is no need #4075

Merged

4 tasks

This pull request was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment