To_lang: my first gem, looking for feedback

Ruby
1

Greetings,

I’ve been working on a library that adds language translation methods to
strings using the Google Translate API. It’s my first time writing a
gem, using RSpec, and documenting with YARD. I’ve gotten it to a point I
feel is worth sharing but I’d love any feedback anyone can provide on my
specs, the documentation, or the code in general. Feel free to be harsh
if it’s bad, cause I’d like to learn and get better.

The source code is on GitHub: GitHub - jimmycuadra/to_lang: Translate Ruby strings and arrays with Google Translate.

Thanks in advance. :slight_smile:

P.S. The rules say no posting advertisements - I hope this doesn’t come
across as one. Apologies if this is against the rules somehow.

2

On Dec 27, 2010, at 14:46 , Jimmy C. wrote:

The source code is on GitHub: GitHub - jimmycuadra/to_lang: Translate Ruby strings and arrays with Google Translate.

Thanks in advance. :slight_smile:

P.S. The rules say no posting advertisements - I hope this doesn’t come
across as one. Apologies if this is against the rules somehow.

That’s different. This is more than allowed. Otherwise I’d be tarred and
feathered by now. :slight_smile:

3

On 27 December 2010 23:46, Jimmy C. [email protected] wrote:

Thanks in advance. :slight_smile:

P.S. The rules say no posting advertisements - I hope this doesn’t come
across as one. Apologies if this is against the rules somehow.

–
Posted via http://www.ruby-forum.com/.

You might be aware, but there is already a Google translate gem:

It seems very well documented and the code is fine IMO.

For the API, I prefer “org_to_dst” to “to_dst_from_org”, but that is
personal.

A detail, but
You define String#{method_missing,respond_to?}, and might override
these methods if they were defined by another library.
You should probably copy the old method with alias, and call it
instead of super.

4

Benoit D. wrote in post #970986:

You might be aware, but there is already a Google translate gem:
GitHub - caius/gtranslate: Google Translate API, written in Ruby

I actually had not seen that one. I was aware of the rtranslate gem, but
gtranslate seems more similar to mine. I did this mostly as a learning
exercise so it’s all right with me that something similar already
exists. My gem also uses a newer version of the GT API.

It seems very well documented and the code is fine IMO.

For the API, I prefer “org_to_dst” to “to_dst_from_org”, but that is
personal.

I thought about this, and you’re right that to_dst_from_org reads
strangely. I will likely add dynamic methods for the reverse-ordered
version in the future so people can use whichever they prefer.

A detail, but
You define String#{method_missing,respond_to?}, and might override
these methods if they were defined by another library.
You should probably copy the old method with alias, and call it
instead of super.

Good advice. Added here:

Thanks very much for your input, Benoit!

5

Aaron P. wrote in post #970994:

When you use File.expand_path, you’re forcing that file to be in a
place relative to this file. That may seem OK, but what you’re really
saying is “I do not want ruby to consult the $LOAD_PATH when requiring”.

Where this really becomes a hindrance is if someone (even yourself) want
to provide an alternate implementation of say “to_lang/connector”, you
could just change the -I flags and provide the correct file. But with
the expand_path form, you’ve prevented any hope of doing that.

I notice you only changed a couple usages of File.expand_path, but
left others alone. Is it somehow correct to use them in these other
places (e.g. string_methods.rb and connector_spec.rb) or were you just
leaving it for me to clean up the rest? I’m not sure how else I’d do it
in the remaining cases, since the specs are not in the load path and
string_methods.rb is already inside “lib/to_lang” which makes me think
it’d end up attempting to load “lib/to_lang/to_lang/codemap.rb” instead
of “lib/to_lang/codemap.rb”. I’m probably just not fully grasping how
the load path works.

Your other notes were very helpful and I have merged your commits into
my master. Thanks a bunch for taking the time to do this. :slight_smile:

6

Jimmy C. wrote in post #971013:

I notice you only changed a couple usages of File.expand_path, but
left others alone. Is it somehow correct to use them in these other
places (e.g. string_methods.rb and connector_spec.rb) or were you just
leaving it for me to clean up the rest? I’m not sure how else I’d do it
in the remaining cases, since the specs are not in the load path and
string_methods.rb is already inside “lib/to_lang” which makes me think
it’d end up attempting to load “lib/to_lang/to_lang/codemap.rb” instead
of “lib/to_lang/codemap.rb”. I’m probably just not fully grasping how
the load path works.

Disregard this. I did some reading on it and understand it better. I
removed all uses of File.expand_path from the project. Thanks again!

7

On Tue, Dec 28, 2010 at 07:46:15AM +0900, Jimmy C. wrote:

Greetings,

I’ve been working on a library that adds language translation methods to
strings using the Google Translate API. It’s my first time writing a
gem, using RSpec, and documenting with YARD. I’ve gotten it to a point I
feel is worth sharing but I’d love any feedback anyone can provide on my
specs, the documentation, or the code in general. Feel free to be harsh
if it’s bad, cause I’d like to learn and get better.

Mostly looks good. I’ve forked your project and pushed a few commits to
clean up (a little):

GitHub - tenderlove/to_lang: A Ruby library that adds language translation methods to strings, backed by the Google Translate API.

I’ll talk about each commit.

do not use expand path when requiring. let $LOAD_PATH decide what is … · tenderlove/to_lang@f75bb72 · GitHub

When you use File.expand_path, you’re forcing that file to be in a
place relative to this file. That may seem OK, but what you’re really
saying is “I do not want ruby to consult the $LOAD_PATH when requiring”.

Where this really becomes a hindrance is if someone (even yourself) want
to provide an alternate implementation of say “to_lang/connector”, you
could just change the -I flags and provide the correct file. But with
the expand_path form, you’ve prevented any hope of doing that.

returning `true` is only necessary if the caller expects that exact v… · tenderlove/to_lang@1aacb77 · GitHub

Explicitly returning true or false usually isn’t a requirement. Very
rarely do people check that the return value of a method is equal to
true or false. Usually it’s used in a statement like this:

if something.start(...)
  ...
end

That statement doesn’t check for the “true” value, but an object that is
“truthy”.

use case / when rather than calling to_s multiple times · tenderlove/to_lang@8eee042 · GitHub

case / when statements use === when evaluating the when part. You can
take advantage that Regexp#=== is the same as Regexp#=~ and reduce the
number of times you call method.to_s.

You should be able to use a similar technique in your method_missing
method.

Overall, good job! Your code looks great! Keep hacking. :slight_smile:

8

On Tue, Dec 28, 2010 at 1:19 AM, Ryan D. [email protected]
wrote:

That’s different. This is more than allowed. Otherwise I’d be tarred and
feathered by now. :slight_smile:

Don’t worry, you are on The List. :wink:

Anyway.

Announcing new releases of gems / Ruby programs / Ruby versions etc.
are at home in ruby-talk, too.

The rules are more about p3||1le enhancements, or unsolicited job
offers.

–
Phillip G.

Though the folk I have met,
(Ah, how soon!) they forget
When I’ve moved on to some other place,
There may be one or two,
When I’ve played and passed through,
Who’ll remember my song or my face.

9

On Tue, Dec 28, 2010 at 05:11:39PM +0900, Jimmy C. wrote:

Disregard this. I did some reading on it and understand it better. I
removed all uses of File.expand_path from the project. Thanks again!

No problem! Glad I could help out. :slight_smile: