[
https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12695500#action_12695500
]
Bryan Duxbury commented on THRIFT-276:
--------------------------------------
This is a revert of a bug fix I just committed a little while ago:
{code}
Index:
/Users/bryanduxbury/projects/thrift/thrift-svn-clean/lib/rb/ext/binary_protocol_accelerated.c
===================================================================
---
/Users/bryanduxbury/projects/thrift/thrift-svn-clean/lib/rb/ext/binary_protocol_accelerated.c
(revision 761704)
+++
/Users/bryanduxbury/projects/thrift/thrift-svn-clean/lib/rb/ext/binary_protocol_accelerated.c
(working copy)
@@ -300,7 +300,7 @@
rb_exc_raise(get_protocol_exception(INT2FIX(BAD_VERSION),
rb_str_new2("No version identifier, old protocol client?")));
}
name = READ(self, version);
- type = read_byte_direct(self);
+ type = rb_thrift_binary_proto_read_byte(self);
seqid = rb_thrift_binary_proto_read_i32(self);
}
{code}
We need to be very careful with this kind of mistake, since when you rename
files you lose patches between when you started your migration and when you
commit it.
Let's put BaseServerTransport in its own file.
I see that you've left files like protocol.rb as a require collection point for
all the real protocol classes. Personally, I'd be just fine if we moved all the
requires up into thrift.rb and got rid of these intermediate level files. We
can group with spacing and comments.
Should Serializer and Deserializer be in separate files?
socket_transport_spec.rb should be named socket_spec. Also, it contains the
spec for ServerSocket, which I think should be moved to another file.
transport_spec should be base_transport_spec? Same with protocol_spec?
Otherwise things are looking good.
> Ruby libraries should have one class per file
> ---------------------------------------------
>
> Key: THRIFT-276
> URL: https://issues.apache.org/jira/browse/THRIFT-276
> Project: Thrift
> Issue Type: Improvement
> Components: Library (Ruby)
> Affects Versions: 0.1
> Reporter: Bryan Duxbury
> Assignee: Michael Stockton
> Priority: Trivial
> Fix For: 0.1
>
> Attachments: thrift-276-v2.patch, thrift-276-v3.patch,
> thrift-276-v4.patch, thrift-276-v5.patch, thrift-276.patch
>
>
> There's no reason for so many of our classes to be lumped into the same file.
> For instance, transport.rb contains 9 classes. They may be short, but
> organizationally, it's superior to have separate class files. Of course, some
> files may contain more than one class per file as appropriate - things like a
> protocol and its factory, for instance, are perfectly acceptable to group.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
