Skip to content
This repository was archived by the owner on May 17, 2018. It is now read-only.

Rename hasNext to hasNextPage and hasPrevious to hasPreviousPage#148

Merged
wyuenho merged 5 commits into
backbone-paginator:masterfrom
vrinek:rename-has-next-previous
Apr 11, 2014
Merged

Rename hasNext to hasNextPage and hasPrevious to hasPreviousPage#148
wyuenho merged 5 commits into
backbone-paginator:masterfrom
vrinek:rename-has-next-previous

Conversation

@vrinek
Copy link
Copy Markdown
Contributor

@vrinek vrinek commented Feb 7, 2014

I noticed a disparity between getNextPage and hasNext functions. I hope that with this fix, they will look a little more symmetrical.

Still TODO (I will need some help with these):

  • make sure the tests pass (I couldn't find anything in the README on how to run the tests)
  • delegate has* to has*Page
  • emit deprecation warnings on has*

@wyuenho
Copy link
Copy Markdown
Member

wyuenho commented Feb 7, 2014

Can you delegate has* to their counterparts has*Page methods and emit a deprecation warning inside the has* methods? This way we can preserve backward compatibility and give users enough time to remove old API calls.

You can read the CONTRIBUTING.md file to find out how to run the tests and other issues on your TODO list.

@wyuenho
Copy link
Copy Markdown
Member

wyuenho commented Feb 18, 2014

Any progress on this PR?

@vrinek
Copy link
Copy Markdown
Contributor Author

vrinek commented Feb 18, 2014

Sorry, I totally forgot about this. I will probably handle it tonight.

@wyuenho
Copy link
Copy Markdown
Member

wyuenho commented Feb 22, 2014

Pinging @vrinek

@vrinek
Copy link
Copy Markdown
Contributor Author

vrinek commented Feb 23, 2014

Pong! Sorry for not updating here. I tried to get the tests running but after npm install I didn't have a grunt executable (expected it to work like ruby's bundler). I tried briefly to figure out why but failed. I'll give it another try today.

@wyuenho
Copy link
Copy Markdown
Member

wyuenho commented Feb 23, 2014

LOL.

You need to globally include grunt-cli with sudo npm install -g grunt-cli

@vrinek
Copy link
Copy Markdown
Contributor Author

vrinek commented Feb 23, 2014

Ok, I got grunt-cli and jsduck. Tests are green and documentation is updated. I will get going with the checklist.

@vrinek
Copy link
Copy Markdown
Contributor Author

vrinek commented Feb 23, 2014

Hmm, how should I emit the deprecated warning? console.warn?

@wyuenho
Copy link
Copy Markdown
Member

wyuenho commented Feb 23, 2014

Yeah really no need to do anything besides updating the code and tests.

Yes console.warn should be sufficient. Make sure you detect the existence of console and console.warn.

@vrinek
Copy link
Copy Markdown
Contributor Author

vrinek commented Feb 23, 2014

Apart from the delegation + warnings, I also duplicated the tests of hasNextPage and hasPreviousPage for their deprecated alternatives. I am not convinced that duplication is a good idea. Please advice on it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to touch this file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that since this is an example, it should be up to date with the non-deprecated functions. Are you sure it should be reverted to use the has* functions?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It will be updated in the backgrid paginator project

Sent from my iPhone

On 24 Feb, 2014, at 5:46 pm, Kostas Karachalios notifications@github.com wrote:

In examples/js/extensions/paginator/backgrid-paginator.js:

@@ -166,8 +166,8 @@
var pageIndex = this.pageIndex;

   if (this.isRewind && currentPage == state.firstPage ||
  •     this.isBack && !collection.hasPrevious() ||
    
  •     this.isForward && !collection.hasNext() ||
    
  •     this.isBack && !collection.hasPreviousPage() ||
    
  •     this.isForward && !collection.hasNextPage() ||
      this.isFastForward && (currentPage == state.lastPage || state.totalPages < 1)) {
    
    I think that since this is an example, it should be up to date with the non-deprecated functions. Are you sure it should be reverted to use the has* functions?


Reply to this email directly or view it on GitHub.

@wyuenho
Copy link
Copy Markdown
Member

wyuenho commented Feb 24, 2014

On second thought, there's no need to touch the tests either. The tests are implicitly testing the deprecated and new methods already.

@vrinek
Copy link
Copy Markdown
Contributor Author

vrinek commented Feb 24, 2014

Ok, I'll revert the tests to their earlier form.

Comment thread lib/backbone-pageable.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will throw a ReferenceError. Use typeof console != 'undefined'

@vrinek
Copy link
Copy Markdown
Contributor Author

vrinek commented Feb 24, 2014

Btw, the tests output is mudded up by the deprecation warnings: https://travis-ci.org/backbone-paginator/backbone-pageable/builds/19485314

@wyuenho
Copy link
Copy Markdown
Member

wyuenho commented Feb 24, 2014

That's fine. Just revert the examples.

wyuenho added a commit that referenced this pull request Apr 11, 2014
Rename hasNext to hasNextPage and hasPrevious to hasPreviousPage
@wyuenho wyuenho merged commit a290658 into backbone-paginator:master Apr 11, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants