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

Change namespace of IO::Buffer to Coolio::Buffer #82

Merged
merged 4 commits into from
Oct 2, 2024

Conversation

Watson1978
Copy link
Contributor

@Watson1978 Watson1978 commented Aug 30, 2024

The IO::Buffer class was introduced at Ruby 3.1.
Ref: https://bugs.ruby-lang.org/issues/18020

The cool.io gem has same name class and it has been conflicted. If you used unknowingly, it will overwrite Ruby's IO::Buffer and it might cause problems.

So, this patch will add namespece that it can be used as Coolio::Buffer.

Related to fluent/fluentd#4619

Types of Changes

  • Bug fix.

Contribution

The `IO::Buffer` class was introduced at Ruby 3.1.
Ref: https://bugs.ruby-lang.org/issues/18020

The cool.io gem has same name class and it has been conflicted.
If you used unknowingly, it will overwrite Ruby's IO::Buffer and it might cause problems.

So, this patch will add namespece that it can be used as `Coolio::IO::Buffer`.
@Watson1978
Copy link
Contributor Author

@ioquatix Could you please review this PR?

Thanks

@ioquatix
Copy link
Member

ioquatix commented Oct 1, 2024

I'm okay with this, but to avoid the namespace confusion, what do you think about renaming it to Coolio::Buffer.

Using Coolio::IO::Buffer means anyone who writes IO inside module Coolio will now get something different.

Imagine this code:

class MyThing
  include Coolio

  def open
    IO.open(...) # This would now refer to `Coolio::IO`
  end
end

@Watson1978
Copy link
Contributor Author

Watson1978 commented Oct 2, 2024

@ioquatix Thank you for your review.
I renamed to Coolio::Buffer and moved iobuffer.c into cool.io dir because it has same namespace.

Thanks.

Copy link
Member

@ioquatix ioquatix left a comment

Choose a reason for hiding this comment

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

This looks great to me, thanks for your effort and implementing the requested changes.

@Watson1978 Watson1978 changed the title Add Coolio namespece to IO::Buffer Change namespace of IO::Buffer to Coolio::Buffer Oct 2, 2024
@ioquatix ioquatix merged commit 0f0e442 into socketry:master Oct 2, 2024
12 checks passed
@ioquatix
Copy link
Member

ioquatix commented Oct 2, 2024

I suppose we can make a minor release for this change.

@Watson1978 Watson1978 deleted the iobuffer branch October 2, 2024 09:12
@ioquatix
Copy link
Member

ioquatix commented Oct 2, 2024

I've released v1.9.0 do you mind testing it and letting me know how it is?

@Watson1978
Copy link
Contributor Author

Thank you!!
OK, I will test cool.io gem v1.9.0 using Fluentd's CI.

If it works as expected, all tests will be successful.
fluent/fluentd#4619

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