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

Fixed. doesn't work with MRI 2.0 #88

Merged
merged 1 commit into from Nov 6, 2018

Conversation

Projects
None yet
2 participants
@sue445
Contributor

sue445 commented Nov 5, 2018

This is regression of 6b9f920

It seems pry-doc supports MRI 2.0.
https://github.com/pry/pry-doc/blob/v0.13.4/pry-doc.gemspec#L23

But required keyword argument is available since MRI 2.1+

Fixed. doesn't work with MRI 2.0
Required keyword argument is available since MRI 2.1+
@@ -17,7 +17,7 @@ class CFile
attr_accessor :file_name
attr_reader :ruby_source_folder
def initialize(file_name:, content:, ruby_source_folder: nil)
def initialize(file_name: nil, content: nil, ruby_source_folder: nil)

This comment has been minimized.

@kyrylo

kyrylo Nov 6, 2018

Member

If kwargs are supported on MRI 2.1+, then how does this work on 2.0? This is also kwargs, right?

This comment has been minimized.

@sue445

sue445 Nov 6, 2018

Contributor

@kyrylo

If kwargs are supported on MRI 2.1+

No. You seem to be confusing Keyword argument and Required keyword argument

Keyword argument available since MRI 2.0+
https://www.ruby-lang.org/en/news/2013/02/24/ruby-2-0-0-p0-is-released/

But required keyword argument available since MRI 2.1+
https://github.com/ruby/ruby/blob/v2_1_0/NEWS#language-changes

def initialize(file_name:, content:, ruby_source_folder: nil) is required keyword argument syntax (for MRI 2.1+)

But def initialize(file_name: nil, content: nil, ruby_source_folder: nil) is (non required) keyword argument syntax (for MRI 2.0+)

See following for details.
https://robots.thoughtbot.com/ruby-2-keyword-arguments#required-keyword-arguments

This comment has been minimized.

@kyrylo

kyrylo Nov 6, 2018

Member

Good points, makes sense, thanks.

@kyrylo

kyrylo approved these changes Nov 6, 2018

@kyrylo kyrylo merged commit 870a968 into pry:master Nov 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sue445 sue445 deleted the sue445:support_ruby_2.0 branch Nov 6, 2018

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