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

Fix undefined method `windows?' #112

Merged
merged 1 commit into from
Nov 11, 2018
Merged

Fix undefined method `windows?' #112

merged 1 commit into from
Nov 11, 2018

Conversation

MatthiasWinkelmann
Copy link
Contributor

This fixes an incompatibility with current pry HEAD, which apparently retired Pry::Platform.

I fear this fix by itself would again be incompatible with current pry release version. But I don't know enough about the respective releases processes, and which strategy you tend to prefer (set dependency to next pry release/respond_to?/etc.).

Feel free to close and/or adapt as you see fit.

This fixes an incompatibility with current pry HEAD, which apparently retired Pry::Platform. 

I fear this fix by itself would again be incompatible with current pry *release* version. But I don't know enough about the respective releases processes, and which strategy you tend to prefer (set dependency to next pry release/```respond_to?```/etc.).

Feel free to close and/or adapt as you see fit.
@kyrylo
Copy link
Collaborator

kyrylo commented Nov 1, 2018

You can use Pry::Platform.windows?still. It’ll emit a warning, though. This way both HEAD and release will be happy.

Alternatively, don’t use this helper and write your own version of that method so you don’t have to rely on this API. Apologies for the mess. I believe the current HEAD API won’t change anymore.

P.S. If I could undo time, I wouldn’t be introducing this helper at all since it’s trivial to write your own check.

@kyrylo
Copy link
Collaborator

kyrylo commented Nov 3, 2018

FWIW, I restored the method and it'll emit a warning. It'll be removed after next Pry release, though.

@vincentwoo
Copy link
Collaborator

We should get this merge and consider a minimum pry version dependency. @MatthiasWinkelmann, can you rebase to trigger a CI rerun?

@vincentwoo
Copy link
Collaborator

Actually I could trigger a rerun and it looks fine. I'll merge and figure out about versioning tomorrow or so.

@vincentwoo vincentwoo merged commit f1e66ca into ConradIrwin:master Nov 11, 2018
@vincentwoo
Copy link
Collaborator

@ConradIrwin can you cut a new release from master?

@ConradIrwin
Copy link
Owner

done! @vincentwoo @kyrylo I've made you both gem maintainers

@vincentwoo
Copy link
Collaborator

@ConradIrwin Great. I'll try to git tag to match your release as well.

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

4 participants