Forum: Ruby to_lang: my first gem, looking for feedback

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: https://github.com/jimmycuadra/to_lang

Thanks in advance. :)

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.

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

> The source code is on GitHub: https://github.com/jimmycuadra/to_lang
>
> Thanks in advance. :)
>
> 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. :)

On 27 December 2010 23:46, Jimmy C. <jimmycuadra@gmail.com> wrote:
>
> Thanks in advance. :)
>
> 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:
https://github.com/caius/gtranslate

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.

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):

  https://github.com/tenderlove/to_lang

I'll talk about each commit.

  https://github.com/tenderlove/to_lang/commit/f75bb7280795c0dbc2b990cf1c020c25b8994cd8

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.

  https://github.com/tenderlove/to_lang/commit/1aacb779448d9886116a426825dea3049ad1e474

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".

  https://github.com/tenderlove/to_lang/commit/8eee042e27d1a12793b55ccedbef3f407ebe7e12

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.  :-)

Benoit Daloze wrote in post #970986:
> You might be aware, but there is already a Google translate gem:
> https://github.com/caius/gtranslate

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: 
https://github.com/jimmycuadra/to_lang/commit/c6966bb6394f79d4383fbf4e9b0861e4cb7df3af

Thanks very much for your input, Benoit!

Aaron Patterson 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. :)

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!

On Tue, Dec 28, 2010 at 1:19 AM, Ryan Davis <ryand-ruby@zenspider.com> 
wrote:
>
> That's different. This is more than allowed. Otherwise I'd be tarred and 
feathered by now. :)

Don't worry, you are on The List. ;)

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 Gawlowski

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.

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. :-)