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.

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

Deprecate usage of should_not raise_error #50

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Deprecate usage of should_not raise_error #50

wants to merge 2 commits into from

Conversation

andrykonchin
Copy link
Member

@andrykonchin andrykonchin commented Dec 5, 2020 •
edited
Loading

Print deprecation message for every example of -> {}.should_not raise_error in specs. I believe it's enough to ensure that new specs don't contain deprecated DSL.

Related issues:

Example

bin/mspec ../spec/core/proc/curry_spec.rb
$ ruby /Users/andrykonchin/projects/mspec/bin/mspec-run ../spec/core/proc/curry_spec.rb
ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-darwin18]
[/ |                   0%                     | 00:00:00]      0F      0E
Deprecation: {}.should_not raise_error breaks code style and is deprecated

Deprecation: {}.should_not raise_error breaks code style and is deprecated
[\ | ==================100%================== | 00:00:00]      0F      0E

Finished in 0.000678 seconds

1 file, 1 example, 2 expectations, 0 failures, 0 errors, 0 tagged

andrykonchin
andrykonchin commented Dec 5, 2020
View reviewed changes
spec/expectations/should_spec.rb Outdated
@@ -5,7 +5,7 @@
before :all do
path = RbConfig::CONFIG['bindir']
exe = RbConfig::CONFIG['ruby_install_name']
file = File.dirname(__FILE__) + '/should.rb'
file = File.dirname(__FILE__) + '/should.rb 2>&1'
Copy link
Member Author

Choose a reason for hiding this comment

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

It prevents running MSpec specs on Windows. Not sure whether it's important.

It would be a little more difficult to check STDERR of the command in this test. Another option is to write a deprecation message to STDOUT instead of STDERR.

Copy link
Member

Choose a reason for hiding this comment

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

I think 2>&1 might actually work on Windows, could you move it to the line below though, so file is still an actual file?

@andrykonchin andrykonchin force-pushed the deprecate-should-not-raise-error branch from eafaf6e to 95a2dcf Compare December 5, 2020 22:39
eregon
eregon reviewed Dec 6, 2020
View reviewed changes
lib/mspec/expectations/should.rb Outdated
@@ -23,6 +23,10 @@ def should_not(matcher = NO_MATCHER_GIVEN)
raise "should_not outside example" unless state
MSpec.actions :expectation, state

if RaiseErrorMatcher === matcher
$stderr.puts "\nDeprecation: ->{}.should_not raise_error breaks code style and is deprecated"
Copy link
Member

Choose a reason for hiding this comment

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

We should show the caller here, so it's easy to know where it comes from.

Copy link
Member

Choose a reason for hiding this comment

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

Could you use MSpec.deprecate (

mspec/lib/mspec/utils/deprecate.rb

Line 2 in 47e3cbf

def self.deprecate(what, replacement)
)?
That will automatically include the caller.

Copy link
Member

Choose a reason for hiding this comment

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

replacement can be e.g. "a matcher to verify the result"

Copy link
Member Author

@andrykonchin andrykonchin Dec 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, will try. I saw this helper but decided not to use it because it has a little bit different purpose - to notify about changing in API or DSL. But here we have a code style issue.

Nevertheless it should work well for our case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

spec/expectations/should_spec.rb Outdated
@@ -5,7 +5,7 @@
before :all do
path = RbConfig::CONFIG['bindir']
exe = RbConfig::CONFIG['ruby_install_name']
file = File.dirname(__FILE__) + '/should.rb'
file = File.dirname(__FILE__) + '/should.rb 2>&1'
Copy link
Member

Choose a reason for hiding this comment

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

I think 2>&1 might actually work on Windows, could you move it to the line below though, so file is still an actual file?

@eregon
Copy link
Member

eregon commented Dec 6, 2020 •
edited
Loading

I think we should start a PR on ruby/spec to fix the warnings, because if we merge and there are hundreds of warnings it will be very annoying for all people running ruby/spec.

@andrykonchin
Copy link
Member Author

There are about 180 examples to change. Will try to fix them soon.

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

@eregon eregon eregon left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@andrykonchin @eregon