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

Refactor/combine #44

Merged
merged 5 commits into from
Feb 13, 2017
Merged

Refactor/combine #44

merged 5 commits into from
Feb 13, 2017

Conversation

Brooooooklyn
Copy link
Contributor

Description

重新定义 combine 并将其重命名为 concat,老的 combine 实现保留。

Related

#5

@Brooooooklyn Brooooooklyn self-assigned this Feb 10, 2017
@Brooooooklyn Brooooooklyn requested a review from Saviio February 10, 2017 07:22
() => new ReactiveDBError(`Token cannot be combined.`)
export const TOKEN_CONCAT_ERR =
(msg?: string) => {
const errMsg = `Token cannot be concat ${ msg ? `due to: ${ msg }` : '' }`
Copy link
Collaborator

Choose a reason for hiding this comment

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

'Token cannot be concated' + `${ msg ? ' due to: ' + msg : '' }.`

}

concat(... selectMetas: Selector<T>[]): Selector<T> {
const equal = selectMetas.every(m =>
Copy link
Collaborator

@Saviio Saviio Feb 11, 2017

Choose a reason for hiding this comment

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

consider:

SELECT * FROM TABLE WHERE id >= 0 SKIP 1 LIMIT 10
SELECT * FROM TABLE WHERE id >= 0 SKIP 2 LIMIT 20
...

these queries will violate constraint of concat method, maybe we should add a term to check if skip mod limit is equal to 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

limit should be equal here

const [ meta ] = metaDatas
const skips = metaDatas
.map(m => m.skip)
.sort()
Copy link
Collaborator

@Saviio Saviio Feb 11, 2017

Choose a reason for hiding this comment

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

consider:

SELECT * FROM TABLE WHER id >= 0 SKIP 10 LIMIT 10
SELECT * FROM TABLE WHER id >= 0 SKIP 20 LIMIT 10
SELECT * FROM TABLE WHER id >= 0 SKIP 30 LIMIT 10
...
SELECT * FROM TABLE WHER id >= 0 SKIP 100 LIMIT 10

after sort => [10, 100, 20, 30, ...]

Copy link
Contributor

Choose a reason for hiding this comment

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

这边 skip 是数字不是字符串吧?

private skip?: number

Copy link
Collaborator

@Saviio Saviio left a comment

Choose a reason for hiding this comment

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

👍

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 95.042% when pulling 8a0c4aa on refactor/combine into bee1a92 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 95.05% when pulling cdbee30 on refactor/combine into bee1a92 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 95.05% when pulling cdbee30 on refactor/combine into bee1a92 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 95.05% when pulling c5d88e8 on refactor/combine into 4d392e6 on master.

@Saviio Saviio merged commit 8c3579e into master Feb 13, 2017
@Saviio Saviio deleted the refactor/combine branch February 13, 2017 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants