You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Bryan Duxbury (JIRA)" <ji...@apache.org> on 2009/01/17 01:47:59 UTC
[jira] Created: (THRIFT-276) Ruby libraries should have one class
per file
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)
Reporter: Bryan Duxbury
Priority: Trivial
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.
[jira] Updated: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael Stockton updated THRIFT-276:
------------------------------------
Attachment: thrift-276-v5.patch
> 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.
[jira] Updated: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael Stockton updated THRIFT-276:
------------------------------------
Attachment: thrift-276.patch
o concentrated require statements in thrift.rb.
o underscored framedTransport, serverTransport and protocolFactory variables and arguments.
o renamed binary_protocol_accelerated to accelerated_binary_protocol.
o added Transport suffix and created individual files for transport classes.o renamed SimpleMongrelHTTPServer to MongrelHTTPServer.
o renamed nonblockingserver.rb to nonblocking_server.rb.
o renamed spec files accordingly.
> 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)
> Reporter: Bryan Duxbury
> Assignee: Michael Stockton
> Priority: Trivial
> Attachments: 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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12695593#action_12695593 ]
Bryan Duxbury commented on THRIFT-276:
--------------------------------------
With v6, rake spec returns
{code}
/usr/local/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:27:in `gem_original_require': no such file to load -- thrift/serializer/serializer (LoadError)
from /usr/local/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:27:in `require'
from ./spec/../lib/thrift.rb:30
from /usr/local/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:27:in `gem_original_require'
from /usr/local/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:27:in `require'
from ./spec/spec_helper.rb:31
from ./spec/base_protocol_spec.rb:20:in `require'
from ./spec/base_protocol_spec.rb:20
from /usr/local/lib/ruby/gems/1.8/gems/rspec-1.1.4/lib/spec/runner/example_group_runner.rb:14:in `load'
from /usr/local/lib/ruby/gems/1.8/gems/rspec-1.1.4/lib/spec/runner/example_group_runner.rb:14:in `load_files'
from /usr/local/lib/ruby/gems/1.8/gems/rspec-1.1.4/lib/spec/runner/example_group_runner.rb:13:in `each'
from /usr/local/lib/ruby/gems/1.8/gems/rspec-1.1.4/lib/spec/runner/example_group_runner.rb:13:in `load_files'
from /usr/local/lib/ruby/gems/1.8/gems/rspec-1.1.4/lib/spec/runner/options.rb:98:in `run_examples'
from /usr/local/lib/ruby/gems/1.8/gems/rspec-1.1.4/lib/spec/runner/command_line.rb:19:in `run'
from /usr/local/lib/ruby/gems/1.8/gems/rspec-1.1.4/bin/spec:4
rake aborted!
{code}
Looks like you forgot to add the serializer dir to the patch.
Other than this omission, things look great.
> 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-v6.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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Kevin Clark (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12695624#action_12695624 ]
Kevin Clark commented on THRIFT-276:
------------------------------------
+1. We look like a real ruby library. Yay.
> 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-v6.patch, thrift-276-v7.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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Kevin Clark (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12694756#action_12694756 ]
Kevin Clark commented on THRIFT-276:
------------------------------------
This generally looks fine, and everything passes. I've no objections, just clarifying questions:
There's still a bunch of classes in transport.rb and protocol.rb. They look like reasonable things to keep together (Sockets mostly in transport, and errors). Was that the idea?
The 'encoding:' lines are for 1.9?
We talked about this a little bit in IRC, but the BinaryProtocolAccelerated name was requested by David for "consistency with php and python". But python's file is called fastbinary.c, so I'm not sure what the deal is. Maybe the Python _classes_ were BPA. Anyway, I'm not outright against the change, but I'd like David's input on it.
Other than that, I'm +1. Good work.
> 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
> Attachments: thrift-276-v2.patch, thrift-276-v3.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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Kevin Clark (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12694823#action_12694823 ]
Kevin Clark commented on THRIFT-276:
------------------------------------
Yup, I'd rather see classes pulled into their own files than having a base.rb.
> 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
> Attachments: thrift-276-v2.patch, thrift-276-v3.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.
[jira] Updated: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael Stockton updated THRIFT-276:
------------------------------------
Attachment: thrift-276-v7.patch
> 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-v6.patch, thrift-276-v7.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.
[jira] Updated: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael Stockton updated THRIFT-276:
------------------------------------
Attachment: thrift-276-v3.patch
v3 patch will work with commits made by Bryan in THRIFT-417.
> 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
> Attachments: thrift-276-v2.patch, thrift-276-v3.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.
[jira] Assigned: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael Stockton reassigned THRIFT-276:
---------------------------------------
Assignee: Michael Stockton
> 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)
> Reporter: Bryan Duxbury
> Assignee: Michael Stockton
> Priority: Trivial
>
> 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.
[jira] Updated: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael Stockton updated THRIFT-276:
------------------------------------
Fix Version/s: 0.1
> 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.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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Kevin Clark (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12694757#action_12694757 ]
Kevin Clark commented on THRIFT-276:
------------------------------------
@Bryan: Yeah, the BPA -> ABP change seems minor and aesthetic, and would break code. I'm not sure how much that bothers me since we're breaking a load of other stuff as well. A little bit though. I think I'd be happy to see that part go and take the rest of the patch.
> 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
> Attachments: thrift-276-v2.patch, thrift-276-v3.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.
[jira] Updated: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael Stockton updated THRIFT-276:
------------------------------------
Attachment: thrift-276-v2.patch
> 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
> Attachments: thrift-276-v2.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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12694755#action_12694755 ]
Bryan Duxbury commented on THRIFT-276:
--------------------------------------
bq. renamed binary_protocol_accelerated to accelerated_binary_protocol
I'm not wild about this. I don't see the "protocol" suffixes (or for that matter, the "transport" suffixes") as a really big benefit. I'd much rather see us use a Thrift::Protocol namespace than focus on ending the names with Protocol.
> 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
> Attachments: thrift-276-v2.patch, thrift-276-v3.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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12694808#action_12694808 ]
Bryan Duxbury commented on THRIFT-276:
--------------------------------------
I think it would be cool to have all the transports and their various base classes pulled out into separate files. It would certainly ease navigation, and make understanding the individual classes hyper-simple.
> 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
> Attachments: thrift-276-v2.patch, thrift-276-v3.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.
[jira] Updated: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael Stockton updated THRIFT-276:
------------------------------------
Patch Info: [Patch Available]
Affects Version/s: 0.1
> 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
> Attachments: 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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Kevin Clark (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12664763#action_12664763 ]
Kevin Clark commented on THRIFT-276:
------------------------------------
I agree entirely. It's only this way for legacy reasons.
> 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)
> Reporter: Bryan Duxbury
> Priority: Trivial
>
> 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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ 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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Kevin Clark (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12695247#action_12695247 ]
Kevin Clark commented on THRIFT-276:
------------------------------------
Oooooh. Real encoding handling. Neat.
This looks much better. I think I'd still prefer to have BPA be BPA, not ABP. If you don't feel strongly about it, could we put it back? Other than that, I'm ready to push.
Bryan?
> 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.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.
[jira] Updated: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael Stockton updated THRIFT-276:
------------------------------------
Attachment: thrift-276-v6.patch
> 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-v6.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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12694802#action_12694802 ]
Michael Stockton commented on THRIFT-276:
-----------------------------------------
bq. I'm not wild about this. I don't see the "protocol" suffixes (or for that matter, the "transport" suffixes") as a really big benefit. I'd much rather see us use a Thrift::Protocol namespace than focus on ending the names with Protocol.
I completely understand. I tried to focus on consistency as much as possible. Although renaming the accelerated protocol to be inline with compact_protocol and binary_protocol is mostly aesthetic, I figured it would be beneficial down the road to have a strong convention. If we reach a consensus either way, I'll be happy.
bq. There's still a bunch of classes in transport.rb and protocol.rb. They look like reasonable things to keep together (Sockets mostly in transport, and errors). Was that the idea?
Correct, I kept Socket, for example, in transport.rb because it didn't feel sensible to have a class named SocketTransport. I can certainly split Socket and ServerSocket out the way Bryan suggests. We could also consider have a base.rb file.
> 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
> Attachments: thrift-276-v2.patch, thrift-276-v3.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.
[jira] Closed: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury closed THRIFT-276.
--------------------------------
Resolution: Fixed
Man, what a patch. Committed. Thanks for all your hard work through many iterations, Michael!
> 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-v6.patch, thrift-276-v7.patch, thrift-276-v8.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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12695352#action_12695352 ]
Michael Stockton commented on THRIFT-276:
-----------------------------------------
I don't feel strong enough to hold this patch back. In return, can you take a look at THRIFT-421? :)
> 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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12695089#action_12695089 ]
Michael Stockton commented on THRIFT-276:
-----------------------------------------
bq. The 'encoding:' lines are for 1.9?
Correct, the magic comment "# encoding: ascii-8bit" sets the default encoding. Without it, spec tests that use literal string comparisons do not match anything read off the wire. By default, everything in ruby1.9 is UTF-8 and we can certainly enforce that with a little more effort. If not, though, it will be pretty easy to merely support Thrift's UTF-8 string type in 1.9 because it's possible to mix-and-match encodings.
stockton ~ % irb1.9 -E ascii-8bit
irb(main):001:0> String.new("foo").encoding.name
=> "ASCII-8BIT"
irb(main):003:0> String.new("foo").encode("utf-8").encoding.name
=> "UTF-8"
stockton ~ % irb1.9
irb(main):001:0> String.new("foo").encoding.name
=> "UTF-8"
> 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
> Attachments: thrift-276-v2.patch, thrift-276-v3.patch, thrift-276-v4.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.
[jira] Updated: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael Stockton updated THRIFT-276:
------------------------------------
Attachment: thrift-276-v8.patch
Removed references to "require 'thrift/protocol'" from the ruby compiler since we only need "require 'thrift'".
> 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-v6.patch, thrift-276-v7.patch, thrift-276-v8.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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12695529#action_12695529 ]
Michael Stockton commented on THRIFT-276:
-----------------------------------------
I was cognizant of the bug you fixed in binary_protocol_accelerated.c -- sorry I missed it. I made the following changes in v6:
o moved BaseServerTransport to its own file.
o migrated all require statements to thrift.rb.
o separated Serializer and Deserializer into different files.
o removed transport.rb, protocol.rb and serializer.rb.
o renamed socket_transport_spec to socket_spec.
o created server_socket_spec file.
o renamed transport_spec and protocol_spec with 'base_' prefix.
> 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.
[jira] Updated: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Michael Stockton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael Stockton updated THRIFT-276:
------------------------------------
Attachment: thrift-276-v4.patch
New patch based on feedback in irc.
> 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
> Attachments: thrift-276-v2.patch, thrift-276-v3.patch, thrift-276-v4.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.
[jira] Commented: (THRIFT-276) Ruby libraries should have one class
per file
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-276?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12694759#action_12694759 ]
Bryan Duxbury commented on THRIFT-276:
--------------------------------------
I think all the Transports in transport.rb should be split out into their own files. I don't think we should be arbitrarily keeping classes together like that - that's what namespaces and directories are for. I tend to use multiple classes per file when the extra classes are trivial and supporting, which is not the case for Socket, for instance.
> 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
> Attachments: thrift-276-v2.patch, thrift-276-v3.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.