Skip to content

Conversation

@bbuchalter
Copy link
Contributor

Hello rubyzip community! 👋

I was thinking about making a small contribution in another area, so I went to run the test suite and noticed this deprecation warning: abstract_output_stream_test.rb:60: warning: '$,' is deprecated.

I learned that setting $, (aka $OUTPUT_FIELD_SEPERATOR) to a non-nil value is now deprecated. This was discussed in https://bugs.ruby-lang.org/issues/14240 and implemented in ruby/ruby@6298ec2.

As a result, I've modified both some test code and application code to assume a world in which this value is always nil.

Since I'm new to this community, and this code was originally almost 20 years ago(!), perhaps we should leave it as is until forced to do otherwise?

Anyway, just a little contribution that I hope is found helpful. Let me know if a CHANGELOG entry would be expected for a change this small.

As discussed in https://bugs.ruby-lang.org/issues/14240 and implemented
in ruby/ruby@6298ec2
setting OUTPUT_FIELD_SEPERATOR to a non-nil value is now depricated.
Since modifying OUTPUT_FIELD_SEPARATOR is deprecated, there's no need
for us to do this in our test.
Since we are using the default behavior of OUTPUT_FIELD_SEPERATOR
anyway, let's allow Ruby to maange that default for us, so if it should
change in the future, we don't have to change.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 95.769% when pulling ab9f546 on bbuchalter:fix_depreciation_warning into e5e3f97 on rubyzip:master.

Copy link
Member

@hainesr hainesr left a comment

Choose a reason for hiding this comment

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

Thanks for this. Sorry it's taken so long for me to look at it.

This looks obviously sensible I think and now would be a good time to implement with a major version bump on the way. Just one question about a now possibly unneeded test...

@hainesr hainesr merged commit c0f2032 into rubyzip:master Jun 7, 2021
@hainesr
Copy link
Member

hainesr commented Jun 7, 2021

Thanks for the PR @bbuchalter - all merged in now.

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.

3 participants