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 spec for Warning[category] = true|false to emit/suppress warnings #806

Merged
merged 1 commit into from
Oct 24, 2020

Conversation

lxxxvi
Copy link
Contributor

@lxxxvi lxxxvi commented Oct 22, 2020

Hello 👋

This time I'll try to cover this one from #745

  •  Added Warning.[] and Warning.[]= to manage emitting/suppressing
    some categories of warnings. Feature #16345 Feature #16420

However, I discovered that suppressing experimental (Warning[:experimental] = false) does not seem to work when invoking a script with ruby -e '...'. In irb it works as expected.

Example:

# Tested with Ruby v2.7.2 on macOS 10.15 (Catalina) and Debian 10 (buster)

# :deprecated
ruby -e 'Warning[:deprecated] = true; $; = ""'  # => -e:1: warning: `$;' is deprecated
ruby -e 'Warning[:deprecated] = false; $; = ""' # =>

# :experimental
ruby -e 'Warning[:experimental] = true; 0 in a'  # => -e:1: warning: Pattern matching is experimental ...
ruby -e 'Warning[:experimental] = false; 0 in a' # => -e:1: warning: Pattern matching is experimental ...

Is this expected, or am I missing something here?

@eregon
Copy link
Member

eregon commented Oct 22, 2020

I think it's because 0 in a is warned at parse time, vs $; warned on assignment.
Using eval() around 0 in a should make it work.

core/warning/warn_spec.rb Outdated Show resolved Hide resolved
@lxxxvi lxxxvi force-pushed the add-spec-for-Warning-square-brackets branch from 6cd1314 to 5ecf14f Compare October 22, 2020 12:19
@lxxxvi
Copy link
Contributor Author

lxxxvi commented Oct 22, 2020

@eregon Once again, thank you for your precious feedback. 🙇

Today I Learned...

_"0 in a is warned at parse time, vs $; warned on assignment."

(and I guess, that's why it works in irb, because each command is put through eval(..)... ?!)


You are right, the warnings disappear with eval('0 in a'), but is this really the intention of Feature 16345?

Given this script.rb

Warning[:experimental] = false
0 in a
ruby ./script.rb
# ./script.rb:2: warning: Pattern matching is experimental, and the behavior may change in future versions of Ruby!

I'd expect that Warning[:experimental] = false suppresses this warning, without wrapping it into eval(..), no?

(Sorry for my annoying questions 😉)

@eregon
Copy link
Member

eregon commented Oct 22, 2020

Feel free to mention that drawback on the issue, but it is expected behavior, parse-time warnings can only be disabled if it's set before parsing of course. And pattern matching emits a parse-time warning.

@lxxxvi lxxxvi force-pushed the add-spec-for-Warning-square-brackets branch 2 times, most recently from cdfe9d4 to cf17c35 Compare October 23, 2020 14:28
@lxxxvi lxxxvi marked this pull request as ready for review October 23, 2020 14:33
@lxxxvi
Copy link
Contributor Author

lxxxvi commented Oct 23, 2020

@eregon 👍 Alright, in that case I think that part of the feature is kind of pointless, even if's technically explainable... 🤷 😀

Thanks for your explanation. I changed the specs accordingly and marked this PR as "Ready for review":

ruby_version_is '2.7' do
describe "Warning.[]" do
it "returns default values for categories :deprecated and :experimental" do
Warning[:deprecated].should == true
Copy link
Member

Choose a reason for hiding this comment

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

Actually deprecated defaults to false in 2.7.2 (but it is true in 2.7.0&1).
I'd suggest using ruby_version_is guards to test that one since it's the big change in 2.7.2:
https://www.ruby-lang.org/en/news/2020/10/02/ruby-2-7-2-released/

Also I just updated the CI to use 2.7.2 so this would actually fail the CI when rebased.

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.

Looks great!

@lxxxvi lxxxvi force-pushed the add-spec-for-Warning-square-brackets branch from cf17c35 to f10289f Compare October 24, 2020 08:53
@lxxxvi lxxxvi force-pushed the add-spec-for-Warning-square-brackets branch from f10289f to e38230c Compare October 24, 2020 09:37
@lxxxvi
Copy link
Contributor Author

lxxxvi commented Oct 24, 2020

@eregon I rebased master and changed the default for :deprecated to false.

However, I had to change the spec n to use ruby_exe(), because the other way it still returned the old default true:

     3: ruby_version_is '2.7.2' do
     4:   describe "Warning.[]" do
     5:     it "returns default values for categories :deprecated and :experimental" do
     6:       ruby_exe('p Warning[:deprecated]').chomp.should == "false"
     7:       ruby_exe('p Warning[:experimental]').chomp.should == "true"
 =>  8:       binding.irb
     9:     end
    10:
    11:     it "raises for unknown category" do
    12:       -> { Warning[:noop] }.should raise_error(ArgumentError, /unknown category: noop/)
    13:     end

irb: warn: can't alias context from irb_context.
2.7.2 :001 > RUBY_VERSION
 => "2.7.2"

2.7.2 :002 > Warning[:deprecated]
 => true

I have no idea why this is 😏 ... do you?

@eregon
Copy link
Member

eregon commented Oct 24, 2020

I have no idea why this is ... do you?

It's because of ruby/mspec@e154fa1, which is intended but I did not think about it in this context.
Using ruby_exe is a good workaround here, especially useful to test global defaults that might be affected by the test harness.

@eregon eregon merged commit 0735383 into ruby:master Oct 24, 2020
@lxxxvi
Copy link
Contributor Author

lxxxvi commented Oct 24, 2020

It's because of ruby/mspec@e154fa1 ...

Thanks for that, I was very confused... 😂

@lxxxvi lxxxvi deleted the add-spec-for-Warning-square-brackets branch October 24, 2020 15:42
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