today's functional test fixes #617

Merged
merged 10 commits into from Oct 11, 2012

Conversation

Projects
None yet
2 participants
@drewfish
Contributor

drewfish commented Oct 11, 2012

No description provided.

@caridy

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Oct 11, 2012

Hmm, a string with "ERROR"? Isn't it better to produce a real HTTP error?

Hmm, a string with "ERROR"? Isn't it better to produce a real HTTP error?

This comment has been minimized.

Show comment Hide comment
@drewfish

drewfish Oct 11, 2012

Owner

Yeah, I didn't like that line. It seemed better to do something rather than have no callback called at all. In practice I expect this else branch to be extremely rare, since most of the time there is an adapter.error() (which is the best way to report errors). Perhaps instead we should just rethrow the error.

Owner

drewfish replied Oct 11, 2012

Yeah, I didn't like that line. It seemed better to do something rather than have no callback called at all. In practice I expect this else branch to be extremely rare, since most of the time there is an adapter.error() (which is the best way to report errors). Perhaps instead we should just rethrow the error.

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Oct 11, 2012

Yeah, I don't understand how/when if (adapter.error) statement will not be true. Do we really need this block?

Yeah, I don't understand how/when if (adapter.error) statement will not be true. Do we really need this block?

This comment has been minimized.

Show comment Hide comment
@drewfish

drewfish Oct 11, 2012

Owner

If we remove it, then we'll start failing on missing adapter.error (which will look like "undefined is not a function").

I think the case of missing adapter.error is when the adapter is provided by the user. (An example is addons/ac/composite, which has its own adapter. While this is part of the mojito package, it is something that actually could have been written by the user themselves.) Anyway, I think it's OK to say that the adapter should have a error(), so I'll remove the else.

Owner

drewfish replied Oct 11, 2012

If we remove it, then we'll start failing on missing adapter.error (which will look like "undefined is not a function").

I think the case of missing adapter.error is when the adapter is provided by the user. (An example is addons/ac/composite, which has its own adapter. While this is part of the mojito package, it is something that actually could have been written by the user themselves.) Anyway, I think it's OK to say that the adapter should have a error(), so I'll remove the else.

@caridy

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Oct 11, 2012

Collaborator

+1

Collaborator

caridy commented Oct 11, 2012

+1

drewfish added a commit that referenced this pull request Oct 11, 2012

Merge pull request #617 from drewfish/perf-func
today's functional test fixes

@drewfish drewfish merged commit 7cfa0ab into YahooArchive:develop-perf Oct 11, 2012

1 check was pending

default The Travis build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment