Skip to content

Conversation

@beltex
Copy link
Contributor

@beltex beltex commented Jun 20, 2014

  • API methods are now chainable, see below for mapping from old methods + README for how it works
  • README updated accordingly
  • Original tests updated accordingly (more should to be added though)
  • Some new methods like settings for hitsPerPage and Page + before option

Methods

Callback only

  • getComments(cb) = comment().top(cb)
  • getLastComments(cb) = comment().recent(cb)
  • getPolls(cb) = poll().top(cb)
  • getLastPolls(cb) = poll.recent(cb)
  • getPosts(cb) = story().poll().top(cb)
  • getLastPosts(cb) = story().poll().recent(cb)
  • getStories(cb) = story().top(cb)
  • getLastStories(cb) = story.recent(cb)

String(s) and callback

  • getCommentsSince(since, cb) = comment().since(marker, cb)
  • getPollsSince(since, cb) = poll().since(marker, cb)
  • getPostsSince(since, cb) = story().poll().since(marker, cb)
  • getStoriesSince(since, cb) = story().since(marker, cb)
  • getItem(id, cb) = item(id, cb)
  • getUser(username, cb) = user(username, cb)
  • getUserComments(username, cb) = author(username).comment().top(cb)
  • getLastUserComments(username, cb) = author(username).comment().recent(cb)
  • getUserCommentsSince(username, since, cb) = author(username).comment().since(marker, cb)
  • getUserPolls(username, cb) = author(username).poll().top(cb)
  • getLastUserPolls(username, cb) = author(username).poll().recent(cb)
  • getUserPollsSince(username, since, cb) = author(username).poll().since(marker, cb)
  • getUserPosts(username, cb) = author(username).story().poll().top(cb)
  • getLastUserPosts(username, cb) = author(username).story().poll().recent(cb)
  • getUserPostsSince(username, since, cb) = author(username).story().poll().since(marker, cb)
  • getUserStories(username, cb) = author(username).story().top(cb)
  • getLastUserStories(username, cb) = author(username).story().recent(cb)
  • getUserStoriesSince(username, since, cb) = author(username).story().since(marker, cb)
  • searchComments(query, cb) = comment().top().search(query, cb)
  • searchLastComments(query, cb) = comment().recent().search(query, cb)
  • searchCommentsSince(query, since, cb) = comment().search(query).since(marker, cb)
  • searchPolls(query, cb) = poll().top().search(query, cb)
  • searchLastPolls(query, cb) = poll().recent().search(query, cb)
  • searchPollsSince(query, since, cb) = poll().search(query).since(marker, cb)
  • searchPosts(query, cb) = story().poll().top().search(query, cb)
  • searchLastPosts(query, cb) = story().poll().recent().search(query, cb)
  • searchPostsSince(query, since, cb) = story().poll().search(query).since(marker, cb)
  • searchStories(query, cb) = story().top().search(query, cb)
  • searchLastStories(query, cb) = story().recent().search(query, cb)
  • searchStoriesSince(query, since, cb) = story().search(query).since(marker, cb)

Object and callback

  • search(obj, cb) = Chain of any tags & page settings + search(query, cb)
  • searchLast(obj, cb) = Chain of any tags & page settings + recent().search(query, cb)
  • searchSince(obj, since, cb) = Chain of any tags & page settings + since().search(query, cb)

@christianbundy
Copy link
Member

Awesome work, I know you put a ton of time and work into this. I like the way that this looks, will you bump the version to 2.0.0? :)

In the future I'd like to change comments().top().search(query, cb) to comments.top.search(query, cb) so that there's one method – I think defining a getter would probably be the best way to do that, but maybe that would be better as 3.0.0. Thanks again for your work, just bump the version and I'll merge it (or you can do it with git merge --ff fix-8).

@beltex beltex merged commit dc97db4 into master Jun 23, 2014
@beltex beltex deleted the fix-8 branch June 23, 2014 21:25
@beltex
Copy link
Contributor Author

beltex commented Jun 23, 2014

Thanks! Version bumped and merge done. Want me to make the release?

So basically like what chai does with should and expect? That is definitely cleaner.

@euank
Copy link
Contributor

euank commented Jun 23, 2014

You could use getters for this and have minimal code changes.

For example, to make this work for hn.story or hn.comment you can simply change line 170 to:

  Object.defineProperty(module.exports, fName, {enumerable: true, get: function () {

However, this has the problem that .story() and .story(id) no longer works because in javascript it has to either be a getter or a function, it can't be both. However, this can be fixed by adding an ".id(id)" function so you could do .story.id ... that would be more coding work though.

@beltex
Copy link
Contributor Author

beltex commented Jun 30, 2014

Sorry about the late response.

So the getter approach as you both mentioned works, but we run into a hitch with story (as @euank mentioned), top, and recent. That is, all 3 have a single optional param, and they can’t be both a getter and a function (as @euank also mentioned).

So here’s a suggestion, have story and story_id(). As for top and recent, make them both getters, and instead move all callbacks to an execute() function. Otherwise, you'd have to have top and top_cb(). So for example:

hn.comment.story.top.execute(cb);

hn.comment.story_id(id).search(query).execute(cb);

So that means that methods like search, just become search(marker). This actually makes the code cleaner underneath, and makes it more explicit from the users perspective that the actual request is being made (executed). But on the other hand its an extra method. Heres the basic idea 9eb8544

What do you think? Is there a better way to do it?

@beltex beltex mentioned this pull request Jul 7, 2014
@beltex
Copy link
Contributor Author

beltex commented Jul 15, 2014

Any thoughts? :)

@christianbundy
Copy link
Member

Sorry, I've been super busy these past few weeks. What if id were its own method, so instead of hn.comment.story_id it were hn.comment.story.id?

@beltex
Copy link
Contributor Author

beltex commented Jul 21, 2014

No worries!

Yeah that works, sounds good.

beltex pushed a commit that referenced this pull request Jul 22, 2014
- As discussed in #9
- Does not enforce correct usage currently (id without story)
- Doesn't handle id before story
@beltex
Copy link
Contributor Author

beltex commented Aug 7, 2014

So are we cool with the execute() method approach? Or leave this for now?

I'm asking because I'm thinking that its probably best to do this change first (switch to getters + execute()) before completing the other open issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants