#101 list objects #103

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@kof
Contributor

kof commented Oct 10, 2012

If this one is ok I will also add #listFiles.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Oct 10, 2012

We need res.on('error', fn) too.

We need res.on('error', fn) too.

This comment has been minimized.

Show comment Hide comment
@kof

kof Oct 10, 2012

Collaborator

right.

Collaborator

kof replied Oct 10, 2012

right.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Oct 10, 2012

It'd be awesome to booleanize and numberize as necessary. E.g.

data.IsTruncated = Boolean(data.IsTruncated);
data.MaxKeys = Number(data.MaxKeys);
data.Contents.forEach(function (object) {
    object.Size = Number(object.Size);
    object.LastModified = new Date(object.LastModified);
});

It'd be awesome to booleanize and numberize as necessary. E.g.

data.IsTruncated = Boolean(data.IsTruncated);
data.MaxKeys = Number(data.MaxKeys);
data.Contents.forEach(function (object) {
    object.Size = Number(object.Size);
    object.LastModified = new Date(object.LastModified);
});

This comment has been minimized.

Show comment Hide comment
@kof

kof Oct 10, 2012

Collaborator

any Ideas how to make it generic and not too slow?

Collaborator

kof replied Oct 10, 2012

any Ideas how to make it generic and not too slow?

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Oct 10, 2012

Hmm, not really. But I think since we know the possible keys returned, just explicitly fixing them on a case-by-case basis is OK.

Hmm, not really. But I think since we know the possible keys returned, just explicitly fixing them on a case-by-case basis is OK.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Oct 10, 2012

Don't update the version in a pull request, I'll do that when I do a release :)

Don't update the version in a pull request, I'll do that when I do a release :)

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Oct 10, 2012

You'll probably want to add these to the multiple-delete test to try and clean up.

You'll probably want to add these to the multiple-delete test to try and clean up.

This comment has been minimized.

Show comment Hide comment
@kof

kof Oct 10, 2012

Collaborator

I think tests should be independent from each other, so I can just clean them up

Collaborator

kof replied Oct 10, 2012

I think tests should be independent from each other, so I can just clean them up

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Oct 10, 2012

Good point.

Good point.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Oct 10, 2012

This is a nice test; well done.

This is a nice test; well done.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Oct 10, 2012

It's not really res (which I would expect to be a HTTP response object); it's more like data I think.

It's not really res (which I would expect to be a HTTP response object); it's more like data I think.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Oct 10, 2012

Contributor

Very nicely done. I left a bunch of little comments but I look forward to merging this. Feel free to amend last commit and force-push.

Contributor

domenic commented Oct 10, 2012

Very nicely done. I left a bunch of little comments but I look forward to merging this. Feel free to amend last commit and force-push.

+ Object.keys(data.Contents[0]),
+ ['Key', 'LastModified', 'ETag', 'Size', 'Owner', 'StorageClass']
+ );
+ client.deleteMultiple(files, function(err, res) {

This comment has been minimized.

Show comment Hide comment
@kof

kof Oct 10, 2012

Contributor

it will never get to this callback, err == null, res.statusCode == 200 ....

do you have the same issue?

@kof

kof Oct 10, 2012

Contributor

it will never get to this callback, err == null, res.statusCode == 200 ....

do you have the same issue?

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Oct 10, 2012

Contributor

No, it's not hanging at all for me... test passes perfectly.

@domenic

domenic Oct 10, 2012

Contributor

No, it's not hanging at all for me... test passes perfectly.

@kof

This comment has been minimized.

Show comment Hide comment
@kof

kof Oct 10, 2012

Contributor

Everything is done except of hanging on ln 354 in knox.test.js

Contributor

kof commented Oct 10, 2012

Everything is done except of hanging on ln 354 in knox.test.js

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Oct 10, 2012

Contributor

I'm ready to merge, since it passes for me and the tests have been flaky for you (which I still really want to fix :-/). Love the normalizeResponse function; very clever. Any last-minute changes you want to make?

Contributor

domenic commented Oct 10, 2012

I'm ready to merge, since it passes for me and the tests have been flaky for you (which I still really want to fix :-/). Love the normalizeResponse function; very clever. Any last-minute changes you want to make?

@kof

This comment has been minimized.

Show comment Hide comment
@kof

kof Oct 10, 2012

Contributor

No :)

Contributor

kof commented Oct 10, 2012

No :)

@domenic domenic closed this in c8a8631 Oct 10, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment