Skip to content

Fixed listFiles() is listing only first 100 files#307

Merged
ksyhoo merged 3 commits intomasterfrom
fix/listFiles_is_listing_only_first_100_files
Nov 5, 2018
Merged

Fixed listFiles() is listing only first 100 files#307
ksyhoo merged 3 commits intomasterfrom
fix/listFiles_is_listing_only_first_100_files

Conversation

@ksyhoo
Copy link
Copy Markdown
Contributor

@ksyhoo ksyhoo commented Oct 28, 2018

[wip]

Resolves #292

Tested locally but feedback would be much appreciated

@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 28, 2018

Codecov Report

Merging #307 into master will decrease coverage by 7.41%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #307      +/-   ##
========================================
- Coverage   39.42%    32%   -7.42%     
========================================
  Files          87     84       -3     
  Lines        2765   2431     -334     
  Branches       10      6       -4     
========================================
- Hits         1090    778     -312     
+ Misses       1674   1653      -21     
+ Partials        1      0       -1

@ksyhoo ksyhoo requested a review from mkucharz October 28, 2018 08:03
@ksyhoo ksyhoo self-assigned this Oct 28, 2018
@mkucharz
Copy link
Copy Markdown
Member

Please add unit test for this case.

})
}

async fetchNextListFiles(hasNext, results){
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.

Missing space before {.


return results.concat(nextFetch.objects.slice(0))
}
else {
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.

You don't need this else here because you have return above.

}
let results = []
const initialFetch = await this.fetch(this.urlFiles(hostingId), {}, headers)
const hasNext = initialFetch.next;
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.

Check this one:

async loadNextPage (response, objects) {

It is very similar, try to use same variable names and constructions.

@ksyhoo ksyhoo requested a review from jonny22094 November 3, 2018 19:51
debug('listFiles')
const headers = {
'X-API-KEY': this.instance.accountKey
try {
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.

i think it is obsolete. You are catching err to throw err. This is async operation just return this.request you don't even need await here.

return objects.concat(nextObjects)
}
return objects
} catch(err) {
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.

Style.

}
return objects
} catch(err) {
throw err
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.

Same thing as above regarding the error handling.

let objects = result.objects
objects = await this.loadNextPage(result, objects)
return objects
} catch (err) {
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.

Same thing as above regarding the error handling.

next: null
})
return hosting
.listFiles(hostingId)
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.

Wrong indent.

Copy link
Copy Markdown
Member

@mkucharz mkucharz left a comment

Choose a reason for hiding this comment

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

LGTM

@ksyhoo ksyhoo merged commit 59d56d0 into master Nov 5, 2018
@ksyhoo ksyhoo deleted the fix/listFiles_is_listing_only_first_100_files branch November 5, 2018 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants