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

Use non deprecated method name #19

Merged
merged 3 commits into from
Dec 6, 2018
Merged

Use non deprecated method name #19

merged 3 commits into from
Dec 6, 2018

Conversation

mathisto
Copy link
Contributor

@mathisto mathisto commented Dec 5, 2018

In prompt.rb:40 use Pry#input_ring vice Pry#input_array, which is being deprecated as per pry/pry#1814

In prompt.rb:40 use Pry#input_ring vice Pry#input_array, which is being deprecated as per pry/pry#1814
@plribeiro3000
Copy link
Owner

Cool. Thanks for the PR @mathisto.

What would happen if the user is locked in an old version of pry? would he still be able to bump jazz_fingers?

@mathisto
Copy link
Contributor Author

mathisto commented Dec 5, 2018

Awww shucks, you are right. This isn't backwards compatible fix. The Pry::Ring class was just added 10 days ago. It was previously History Array. The newest version of pry will throw a warning about the deprecation.
image
Based on the version of pry you have in your gemspec, you might want to just reject the PR, or hold onto it for when/if you update.

P.S. Ignore the fact I tried ls in the pry console. It a weird quirk of muscle memory.

@plribeiro3000
Copy link
Owner

@mathisto believe we can work on top of what we already have here.

What if we check if the object respond to input_ring and based on it use one or the other?

The newest version of pry renames the history array to ring. This selectively responds to the newest version of the method.
@@ -37,7 +37,9 @@ def name
end

def line_number(pry)
"[#{bold_text(pry.input_array.size)}]"
pry.respond_to? :input_ring ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lint/LiteralAsCondition: Literal :input_ring appeared as a condition.
Style/MultilineTernaryOperator: Avoid multi-line ternary operators, use if or unless instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok Hound, I'll bow to your demands.

@mathisto
Copy link
Contributor Author

mathisto commented Dec 6, 2018

Great call with respond_to?. This works nicely and will support future changes to pry version.

Taking the hounds advice. Multi-line ternaries are apparently frowned upon.
@plribeiro3000
Copy link
Owner

LGTM! Thanks for bearing with me on this!

@plribeiro3000 plribeiro3000 merged commit 6ca5232 into plribeiro3000:master Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants