erunion / showtimes

[deprecated] a movie showtimes API
MIT License
52 stars 14 forks source link

Adding query param to getMovies and getTheaters methods. #33

Closed chrux closed 8 years ago

chrux commented 8 years ago
erunion commented 8 years ago

Overall this is pretty awesome, but I'm a bit iffy on having query be the second argument in these APIs after the callback. If you could swap them out, I can prep this for a 3.0.0 release since it's a breaking change.

Also, can you run npm run lint (might need to npm install -g standard) to fix some minor code styling issues.

chrux commented 8 years ago

@erunion working on this.

chrux commented 8 years ago

To avoid breaking it what do you think about using polymorphism (overloading) in this method? @erunion

chrux commented 8 years ago

Lol, not possible in JS, forget it :)

erunion commented 8 years ago

Well you can maybe do something where if cb is present, but is a string, consider it as the query, and then treat the second argument as the real cb. That'd keep some backwards compatibility.

Those kind of hidden/alternate APIs are always kind of funky, though.

Up to you, though. However way you want to do it, I'm cool with. I'd just prefer to have the callback as the last argument for the user to pass in, since it makes the code look a bit nicer.

chrux commented 8 years ago

@erunion sounds awesome, let me make the change then.

chrux commented 8 years ago

Check this last commit and let me know if we can leave the cb pass by arguments? so we can avoid using cb as param and using it as query which has nothing to do with the callback, this is just a way to avoid misunderstanding.

erunion commented 8 years ago

We'll need to update the documentation a bit to make it clear that those methods have some alternate/hidden APIs, but I think that change looks great.

erunion commented 8 years ago

Tagged to 2.1.0.