Jens-G commented on PR #3656: URL: https://github.com/apache/thrift/pull/3656#issuecomment-5039271629
### Code review No issues found. Checked for bugs and CLAUDE.md compliance. Two suggestions, not blockers: 1. The new guard in `Thrift::Socket#open` doesn't propagate to `SSLSocket#open` or `UNIXSocket#open`, which override `open` without calling `super` or checking `open?` (the PR description notes this scoping is intentional): https://github.com/apache/thrift/blob/70b530df05e3a5ded32d55970c97afbf3dde198a/lib/rb/lib/thrift/transport/ssl_socket.rb#L36-L39 https://github.com/apache/thrift/blob/70b530df05e3a5ded32d55970c97afbf3dde198a/lib/rb/lib/thrift/transport/unix_socket.rb#L32-L36 Moving the new spec into the shared example group both `ssl_socket_spec.rb` and `unix_socket_spec.rb` already include (`it_should_behave_like "a socket"`) would exercise the same property across all three transports: https://github.com/apache/thrift/blob/70b530df05e3a5ded32d55970c97afbf3dde198a/lib/rb/spec/socket_spec_shared.rb#L22-L26 2. This PR changes observable behavior (a second `open` now raises `ALREADY_OPEN` instead of silently succeeding). THRIFT-5942 and THRIFT-6098 — prior changes to this same exception behavior — both added a note to `lib/rb/README.md`'s Breaking Changes / Migration Notes sections; consider doing the same here: https://github.com/apache/thrift/commit/73123f9d44c33da5d92b9cdfdc183256beb7b889 https://github.com/apache/thrift/commit/bc2dd8fdb5b2aeecaa66b737db7de6c019a452f2 🤖 Generated with [Claude Code](https://claude.ai/code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
