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

prettify `shellwords` usage #951

Merged
merged 2 commits into from Jul 3, 2013

Conversation

@rctay
Copy link
Contributor

commented Jun 20, 2013

Since shellwords extends String with shellescape and shellsplit (corresponding to Shellwords::shellwords), we may omit blocks to map that just invoke those methods.

The &:inst_meth works on 1.8 too.

@coveralls

This comment has been minimized.

Copy link

commented Jun 20, 2013

Coverage Status

Coverage increased (+0%) when pulling 71052bc on rctay:rc/shellwords into dee12fc on rspec:master.

@samphippen

This comment has been minimized.

Copy link
Member

commented Jun 20, 2013

I think this is good. @myronmarston I just wanted to double check. This doesn't need a changelog entry because it's just simple internal refactoring?

@JonRowe

This comment has been minimized.

Copy link
Member

commented Jun 22, 2013

I believe we want to hold off on this to 3.0.0, I could be wrong but I think 1.8.6 has something to say about shellwords...

@rctay

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2013

I gathered from the Gemfile that rspec-core requires at least 1.8.7, did I miss anything?

@JonRowe

This comment has been minimized.

Copy link
Member

commented Jun 23, 2013

Master (Rspec 3) supports 1.8.7 and 1.9.2 and above but RSpec 2.14
nominally (to the point of it will parse and mostly run) supports 1.8.6

On Sunday, 23 June 2013, Tay Ray Chuan wrote:

I gathered from the Gemfile that rspec-core requires at least 1.8.7, did I
miss anything?


Reply to this email directly or view it on GitHubhttps://github.com//pull/951#issuecomment-19870580
.

@rctay

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2013

Ok, as you pointed out in c8b6587, this may not work on 1.8.6.

@JonRowe

This comment has been minimized.

Copy link
Member

commented Jun 23, 2013

Yep, FYI:

1.8.6 :001 > require 'shellwords'
 => true
1.8.6 :002 > "".shellsplit
NoMethodError: undefined method `shellsplit' for "":String
1.8.6 :003 > Shellwords.shellwords ""
 => []

So this is a master merge only...

@samphippen

This comment has been minimized.

Copy link
Member

commented Jul 3, 2013

I've come to the following conclusions: this is a good change for RSpec 3.0.0 and it doesn't need a Changelog entry. @rctay thanks for the contribution!

samphippen added a commit that referenced this pull request Jul 3, 2013

Merge pull request #951 from rctay/rc/shellwords
prettify `shellwords` usage

@samphippen samphippen merged commit e1ab4e1 into rspec:master Jul 3, 2013

1 check passed

default The Travis CI build passed
Details
@JonRowe

This comment has been minimized.

Copy link
Member

commented Jul 3, 2013

❤️

@rctay

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2013

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.