Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle situation when prev and next orders are empty; fix#225 #226

Merged
merged 2 commits into from Oct 15, 2018
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Prev

fixed codereview notices and CI report

  • Loading branch information
AlexeyBarabash committed Oct 13, 2018
commit e3e9577f26d4a3594f33f86d012dd7fc65565620
@@ -44,16 +44,18 @@ const getPrevOrderFromNextOrder = (lastNumber, order) => {
* Returns current bookmark order based on previous and next bookmark order.
* @param {String} prevOrder
* @param {String} nextOrder

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Oct 12, 2018

Member

please document parentOrder in these comments (using jsdoc)

This comment has been minimized.

Copy link
@AlexeyBarabash

AlexeyBarabash Oct 13, 2018

Author Contributor

thanks, this was fixed

* @param {String} parentOrder
* @returns {String}
*/
module.exports.getBookmarkOrder = (prevOrder, nextOrder, parentOrder) => {
let prevOrderSplit = prevOrder.split('.')
let nextOrderSplit = nextOrder.split('.')

if (prevOrderSplit.length === 1 && nextOrderSplit.length === 1) {
if (!parentOrder) {

This comment has been minimized.

Copy link
@bridiver

bridiver Oct 12, 2018

Contributor

we should handle the case where all 3 are blank (first node added) - we really have no use or need for the base order in the client and I think we should encapsulate all the logic here so the client doesn't have to know or care about how the order works, so I think we should store it in the lib and use it for this case

This comment has been minimized.

Copy link
@bridiver

bridiver Oct 12, 2018

Contributor

we have a workaround for now in brave-core so that could be done in a follow up

throw new Error(`Invalid previous and next orders: ${prevOrderSplit}, ${nextOrderSplit} and ${parentOrder}`)
throw new Error(`Invalid previous, next and parent orders: ${prevOrderSplit}, ${nextOrderSplit} and ${parentOrder}`)
} else {
return parentOrder + '.1';
return parentOrder + '.1'
}
}
let order = ''
@@ -18,8 +18,8 @@ test('test bookmarkUtil', (t) => {
t.equals(bookmarkUtil.getBookmarkOrder('2.0.8.10', '2.0.8.15.1'), '2.0.8.10.1')
t.equals(bookmarkUtil.getBookmarkOrder('2.0.8.10', '2.0.8.11.1'), '2.0.8.10.1')
t.equals(bookmarkUtil.getBookmarkOrder('2.0.8.10.0.1', '2.0.8.15.1'), '2.0.8.10.0.2')
t.equals(bookmarkUtil.getBookmarkOrder('', '', '2.0.9'), '2.0.9.1')
t.throws(() => bookmarkUtil.getBookmarkOrder('', '', ''), /Invalid previous, next and parent/, 'requires at least one argument')
t.throws(() => bookmarkUtil.getBookmarkOrder('', '', '2.0.9'), '2.0.9.1')
t.throws(() => bookmarkUtil.getBookmarkOrder('', '2.0.0'), /Invalid input order/)
t.throws(() => bookmarkUtil.getBookmarkOrder('2.0.0', ''), /Invalid input order/)
})
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.