bellycard / napa

A simple framework for building APIs with Grape
Other
329 stars 72 forks source link

Indefinite Article Support in API Generation #211

Closed hstrowd closed 9 years ago

hstrowd commented 9 years ago

Adds support for using 'an' for APIs whose name starts with a vowel.

shaqq commented 9 years ago

did you consider using the logic in this gem:

https://github.com/rossmeissl/indefinite_article/blob/master/lib/indefinite_article.rb

hstrowd commented 9 years ago

@shaqq: I hadn't come across this. Thanks for bringing it to my attention. I'll make an update to use this gem instead. Thanks.

umtrey commented 9 years ago

+1 from me, but rebase with master to get rid of these awful merge conflicts ugh how dreadful

heymackey commented 9 years ago

My 2 cents, do we really want to add a 3rd party gem dependency? It limits the flexibility of adding additional words that might need an an to the indefinite_article gem, vs having some additional configuration list that would apply, sort of like how ActiveRecord Inflections work.

Just wanted to throw that out there.

umtrey commented 9 years ago

This is just on API generation - manually changing things after the fact is probably expected. This just lowers the number of times we have to manually change things after we run the generator. There's another gem that's actually maintained that's doing the same thing, so why not use it?

shaqq commented 9 years ago

I'm with Mackey on this one. I'd rather copy/pasta the logic from that gem since it's not that much. Also, we have enough issues with third party gems and setting versions on them, and I'd rather not add to the list. 

Shaker Islam - Platform Engineer shaker@bellycard.com www.bellycard.com | @shakerdev

On Fri, May 22, 2015 at 9:13 AM, Trey Springer notifications@github.com wrote:

This is just on API generation - manually changing things after the fact is probably expected. This just lowers the number of times we have to manually change things after we run the generator. There's another gem that's actually maintained that's doing the same thing, so why not use it?

Reply to this email directly or view it on GitHub: https://github.com/bellycard/napa/pull/211#issuecomment-104670879

heymackey commented 9 years ago

Like I said, just a thought. If it's as trivial as having smarter code generation, @hstrowd's original approach is simpler without the dependency.

umtrey commented 9 years ago

The only dependencies that this gem has are activesupport, i18n, and rake, and they're not version dependent. If we want the simplest solution, then what we had before was best - I hate just using the code wholesale without doing the awesome thing and getting the gem that Ross made baked in.

If we just use the code, we will have to attribute it at least and add the license, which have their own hangups. It's a nice little gem that already exists, that's all. We could even make it load only in development or test, which are the only places that would be using the generators, if we're worried about how burdened this is at deployment. Or, just go with the much simpler, dumber rule addition that @hstrowd already had.

heymackey commented 9 years ago

@umtrey makes a good point about licensing if we use the code as is from indefinite_article.

I don't know if you can specific (RAILS|RAKE)_ENV loading within a gem, as the development dependency stuff in the gemspec is for gem development, not the gem being in used in a Rails.env.development? context.

Napa has quite a few dependencies, which is what I think @shaqq is referring to, and needing to keep those up-to-date.

Either way, this discussion is "6 one way, half-dozen another" in terms of the functionality is concerned. I'd err on the side simpler and under Napa's control over 3rd party dependency, but don't want to hold up someone else's productivity at the cost of that bias. We can always take the gem out and add a more full-featured Napa solution if we get bit by it.

heymackey commented 9 years ago

Also, @hstrowd ++ for even thinking of adding this!

shaqq commented 9 years ago

yup, i'm in favor of merging this right now. make it work, make it better :)

+1

hstrowd commented 9 years ago

I went ahead and resolved the conflicts. I'm not terribly opinionated regarding whether or not we use the gem or a simplified method like the one I originally had. I haven't felt the cost of managing and keeping these gems up to date, so I may be underestimating the cost of this, but I was glad to find a gem that did just what I was looking for.

heymackey commented 9 years ago

+1. Thanks @hstrowd, @shaqq and @umtrey!