Skip to content

Ruby 2.7 compatibility fixes #93

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

Merged
merged 6 commits into from
Sep 3, 2020
Merged

Conversation

perezperret
Copy link
Contributor

@perezperret perezperret commented Aug 30, 2020

Fixes a couple of issues that are breaking ruby 2.7 compatibility

  • Uses keyword arguments instead of hash in a call to String#encode. (This fixes deprecation warnings)
  • Uses external_encoding option on CSV read and manually encodes back to the source encoding after CSV read. This took some experimentation, since CSV::read has changed the encoding behavior.
  • Added 2.7 to CI

I made some changes to the Gemfile that seemed to be needed. I'm not sure if I agree with keeping the lock file out of version control, is this something worth discussing?

I also propose we add other versions of ruby to the CI, just in case.

@pcreux
Copy link
Owner

pcreux commented Sep 3, 2020

Thank you so much for your contribution @perezperret !

I'm not sure if I agree with keeping the lock file out of version control, is this something worth discussing?

I believe that we're not supposed to check in Gemfile.lock in ruby gems repositories. See Yehuda's recommendation from 10 years ago. :)

I also propose we add other versions of ruby to the CI, just in case.

We could potentially do that. Ruby 2.7 is the first version with breaking changes since 2.3 (and I believe that it works just fine on 2.1 and 2.2). So while I'm not opposed to it, I'm not sure that it would bring much value.

I'm going to merge this PR and release a new version. Thanks again!

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.

2 participants