HacktheUniverse / star-api

All these stars belong to you
http://star-api.herokuapp.com/
80 stars 16 forks source link

Resources refactor #39

Closed nicholalexander closed 6 years ago

nicholalexander commented 9 years ago

Start for the structure of the new v2 refactor. Doesn't include search or min/max filters on elements, that's for the next incremental round.

nicholalexander commented 9 years ago

@surenm hey man - what do you think of this? i started working on tests based on this but don't wanna go further on that front until this gets reviewed. (generally speaking I don't think we should merge our own pull requests, best practice and all.)

surenm commented 9 years ago

Will review this today!

On Fri, Nov 14, 2014 at 11:57 AM, nichol alexander <notifications@github.com

wrote:

Assigned #39 https://github.com/HacktheUniverse/star-api/pull/39 to @surenm https://github.com/surenm.

— Reply to this email directly or view it on GitHub https://github.com/HacktheUniverse/star-api/pull/39#event-193461454.

Cheers, ~Suren.

http://suren.in http://twitter.com/suren skype: pingsuren

surenm commented 9 years ago

Bugfix: The original url pattern in routes was buggy. we did :resource/:name. name doesnt match labels with space in them. So we have to make it :resource/*name

Suggestion 1: This is alright. But we are having a lot of code duplication. One way to handle this is have a base controller that all these inherit from. Only override the methods those need to be overridden? If this sounds right, please to make the change!

surenm commented 9 years ago

Resolves #22

nicholalexander commented 9 years ago

I thought about that but was worried it would be overly complicated because we would override basically everything as there are different labels for the index of each resource - check the routes. But I also don't know if that's best strategy.

I personally don't mind the duplication in the name of readability and simplicity to read the code. Your guys' call! On Nov 14, 2014 12:34 PM, "Surendran Mahendran" notifications@github.com wrote:

This is alright. But we are having a lot of code duplication. One way to handle this is have a base controller that all these inherit from. Only override the methods those need to be overridden?

Reply to this email directly or view it on GitHub https://github.com/HacktheUniverse/star-api/pull/39#issuecomment-63100290 .

surenm commented 9 years ago

Ok, lets leave it like that for now. Can you fix the routes pattern though?

On Fri, Nov 14, 2014 at 12:44 PM, nichol alexander <notifications@github.com

wrote:

I thought about that but was worried it would be overly complicated because we would override basically everything as there are different labels for the index of each resource - check the routes. But I also don't know if that's best strategy.

I personally don't mind the duplication in the name of readability and simplicity to read the code. Your guys' call! On Nov 14, 2014 12:34 PM, "Surendran Mahendran" notifications@github.com wrote:

This is alright. But we are having a lot of code duplication. One way to handle this is have a base controller that all these inherit from. Only override the methods those need to be overridden?

Reply to this email directly or view it on GitHub < https://github.com/HacktheUniverse/star-api/pull/39#issuecomment-63100290>

.

— Reply to this email directly or view it on GitHub https://github.com/HacktheUniverse/star-api/pull/39#issuecomment-63101708 .

Cheers, ~Suren.

http://suren.in http://twitter.com/suren skype: pingsuren

surenm commented 9 years ago

And we also need to start using jbuilder.json to control what goes out of the API. Some one was confused with the timestamps in the json response. This is also good because we want to introduce new calculated fields into the response.