Skip to content
This repository has been archived by the owner on Apr 4, 2022. It is now read-only.

implement #138 since option for collection.getTotalRecords #142

Closed
wants to merge 3 commits into from
Closed

implement #138 since option for collection.getTotalRecords #142

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e2a12d7
implement #138 since option for collection.getTotalRecords
elelay Sep 16, 2016
2e591fc
pass since in options for collection.getTotalRecords
elelay Sep 27, 2016
f2e63bc
improve code style and formatting; don't pass since to _collOptions
elelay Sep 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/collection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,18 @@ export default class Collection {
*
* @param {Object} [options={}] The options object.
* @param {Object} [options.headers] The headers object option.
* @param {String} [options.since] The since ETag option.
* @return {Promise<Number, Error>}
*/
getTotalRecords(options={}) {
const { headers } = this._collOptions(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

So that means since is passed to _collOptions and merged into the instance options. I don't think that it's a problem per se, but explicitely removing it may be nicer:

getTotalRecords(options={}) {
  const { since, ...otherOptions } = options;
  const { headers } = this._collOptions(otherOptions);
  let path = endpoint("record", this.bucket.name, this.name);
  if (since) {
    path += "?since="+options.since;
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, the const { since, ...otherOptions } = options instruction breaks the firefox build:
https://travis-ci.org/Kinto/kinto-http.js/jobs/163077307#L959

Maybe this issue: browserify/module-deps#93

Copy link
Contributor

@n1k0 n1k0 Sep 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Sigh. Oh well.

Edit: just to be clear, that's because this is es7 and neither firefox and babel without specific transforms support this syntax.

let path = endpoint("record", this.bucket.name, this.name);
if(options.since){
Copy link
Contributor

@n1k0 n1k0 Sep 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

nit: space after if and before {

path += "?since="+options.since;
Copy link
Contributor

@n1k0 n1k0 Sep 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

It may be nice to reuse the utils.qsify function here.

Also nit: space around operand

}
return this.client.execute({
method: "HEAD",
path: endpoint("record", this.bucket.name, this.name),
path: path,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: just path is enough.

Copy link
Contributor

@n1k0 n1k0 Sep 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I insist on this, pretty please:

return this.client.execute({
    method: "HEAD",
    path,
    headers,
  }, {raw: true})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used path directly. Is there something else to do?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's basically the difference between:

return this.client.execute({
    method: "HEAD",
    path: path,
    headers,
  }, {raw: true})

and

return this.client.execute({
    method: "HEAD",
    path,
    headers,
  }, {raw: true})

headers
}, {raw: true})
.then(({headers}) => parseInt(headers.get("Total-Records"), 10));
Expand Down
12 changes: 12 additions & 0 deletions test/collection_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,18 @@ describe("Collection", () => {
return getBlogPostsCollection().getTotalRecords()
.should.become(42);
});

it("should pass the since parameter", () => {
sandbox.stub(client, "execute").returns(Promise.resolve());

getBlogPostsCollection().getTotalRecords({since: "ETAG"});

sinon.assert.calledWithMatch(client.execute, {
method: "HEAD",
path: "/buckets/blog/collections/posts/records?since=ETAG",
}, {raw: true});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


});

/** @test {Collection#getData} */
Expand Down