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

feat(bindings/ruby): Add I/O class for Ruby #5354

Merged
merged 11 commits into from
Dec 6, 2024
Merged

Conversation

erickguan
Copy link
Contributor

Which issue does this PR close?

Part of #5227.

Rationale for this change

An incremental update to Ruby binding work. This PR implements an IO like class.

Many TODOs:

  • Encoding
  • More functions for this IO class

Are there any user-facing changes?

No.

@erickguan erickguan changed the title Ruby binding A IO class for Ruby binding Nov 23, 2024
@erickguan erickguan changed the title A IO class for Ruby binding An I/O class for Ruby binding Nov 23, 2024
bindings/ruby/Cargo.toml Outdated Show resolved Hide resolved
bindings/ruby/src/lib.rs Outdated Show resolved Hide resolved
bindings/ruby/test/io_test.rb Outdated Show resolved Hide resolved
bindings/ruby/lib/opendal_ruby/io.rb Show resolved Hide resolved
Copy link
Contributor Author

@erickguan erickguan left a comment

Choose a reason for hiding this comment

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

Fixed.

bindings/ruby/src/lib.rs Outdated Show resolved Hide resolved
bindings/ruby/Cargo.toml Outdated Show resolved Hide resolved
bindings/ruby/test/io_test.rb Outdated Show resolved Hide resolved
bindings/ruby/lib/opendal_ruby/io.rb Show resolved Hide resolved
@erickguan erickguan requested a review from Xuanwo December 4, 2024 10:37
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, thank you @erickguan!

@@ -18,7 +18,7 @@
[package]
name = "opendal-ruby"
publish = false
version = "0.1.11"
version = "0.1.12"
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to update it in this PR; the release manager will take care of it during the release.

Copy link
Contributor Author

@erickguan erickguan Dec 6, 2024

Choose a reason for hiding this comment

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

Thanks for future release manager!! Reverted


Gem::Specification.new do |spec|
spec.name = "opendal"
spec.version = OpenDAL::VERSION
# RubyGems integrates and expects `cargo`.
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit surprising to use json and need to call cargo for this. This might make things a bit complicated during the release process. However, it's fine for now; we can address it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you.

If my memory checks out, RubyGems can also read a version file. At least I can do this manually. But I didn't find Cargo.toml supporting this.

@erickguan erickguan requested a review from Xuanwo December 6, 2024 10:24
@erickguan
Copy link
Contributor Author

Thanks for the review. I am 2-3 PRs away from achieving the issue.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you @erickguan for working on this, let's move!

@Xuanwo Xuanwo changed the title An I/O class for Ruby binding feat(bindings/ruby): Add I/O class for Ruby Dec 6, 2024
@Xuanwo Xuanwo merged commit 2c2d826 into apache:main Dec 6, 2024
33 checks passed
@erickguan erickguan deleted the ruby-binding branch December 6, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants