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 listPath API for size node API in #1272 #1297

Merged
merged 20 commits into from May 30, 2019

Conversation

jmwski
Copy link
Contributor

@jmwski jmwski commented May 15, 2019

This change is both npm test and npm install clean.

Description

This is a refactor that #1239 depends on.

Scenarios Tested

Modified tests and existing test suite pass.

Sample Commands

@googlebot googlebot added the cla: yes Manual indication that this has passed CLA. label May 15, 2019
@jmwski jmwski requested a review from ryanpbrewster May 15, 2019 19:31
@jmwski jmwski requested a review from fredzqm May 15, 2019 19:33
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 62.643% when pulling 45fd7b6 on wyszynski/list-remote-refactor-v2 into fd143c4 on master.

@coveralls
Copy link

coveralls commented May 15, 2019

Coverage Status

Coverage increased (+0.08%) to 61.473% when pulling 8a8a90d on wyszynski/list-remote-refactor-v2 into 08f053e on master.

src/database/listRemote.ts Outdated Show resolved Hide resolved
src/test/database/fakeListRemote.spec.ts Show resolved Hide resolved
src/test/database/fakeListRemote.spec.ts Outdated Show resolved Hide resolved
src/test/database/fakeListRemote.spec.ts Outdated Show resolved Hide resolved
src/test/database/fakeListRemote.spec.ts Outdated Show resolved Hide resolved
src/test/database/fakeListRemote.spec.ts Show resolved Hide resolved
src/test/database/listRemote.spec.ts Outdated Show resolved Hide resolved
src/test/database/listRemote.spec.ts Outdated Show resolved Hide resolved
src/test/database/listRemote.spec.ts Show resolved Hide resolved
src/test/database/removeRemote.spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@fredzqm fredzqm left a comment

Choose a reason for hiding this comment

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

A few nits. Really close.

src/test/database/fakeListRemote.spec.ts Outdated Show resolved Hide resolved
src/test/database/fakeListRemote.spec.ts Outdated Show resolved Hide resolved
src/test/database/listRemote.spec.ts Show resolved Hide resolved
@jmwski jmwski requested review from fredzqm and bkendall and removed request for fredzqm May 18, 2019 00:32
@jmwski jmwski requested a review from fredzqm May 18, 2019 00:32
Copy link
Contributor

@bkendall bkendall left a comment

Choose a reason for hiding this comment

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

Couple nits, but looks good!

src/test/database/fakeListRemote.spec.ts Outdated Show resolved Hide resolved
src/test/database/listRemote.spec.ts Show resolved Hide resolved
@jmwski jmwski requested a review from bkendall May 30, 2019 02:14
Copy link
Contributor

@fredzqm fredzqm left a comment

Choose a reason for hiding this comment

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

Looks Good!
I think this was ready a few days ago, but did not catch my attention.
Could you add me to the assignee so I get notified?

@jmwski jmwski merged commit 4f4acf4 into master May 30, 2019
@jmwski jmwski deleted the wyszynski/list-remote-refactor-v2 branch May 30, 2019 19:08
path: string,
numSubPath: number,
startAfter?: string,
timeout?: number
Copy link
Contributor

Choose a reason for hiding this comment

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

let's name this timeoutMs to indicate what the semantics are

path: string,
numChildren: number,
startAfter?: string,
timeout?: number
Copy link
Contributor

Choose a reason for hiding this comment

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

timeoutMs

keys = keys.slice(0, numChildren);
return keys;
}
return [];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can reduce complexity by saying let keys = Object.keys(d || {}) (or by switching this to an early return)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants