Skip to content

Conversation

albertyw
Copy link
Contributor

@albertyw albertyw commented Oct 3, 2015

No description provided.

@albertyw albertyw mentioned this pull request Oct 6, 2015
@albertyw albertyw changed the title More refactoring More refactoring and add tests Oct 6, 2015
@albertyw albertyw force-pushed the refactor branch 6 times, most recently from 5350ba1 to 9f9420b Compare October 7, 2015 00:16
@jkodumal
Copy link
Contributor

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably check if the status is >= 300, since other 2xx responses could still indicate that everything is okay. (I realize that this is unchanged from before, but I just noticed it here.)

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 changed this to res.status/100 != 2 since I assume 100 Continue responses are also not accepted. I also fixed 2 other places where it was checking for 200 specifically.

@jkodumal
Copy link
Contributor

I saw a few sporadic integration test failures when I bumped the REST wrapper to this branch. Investigating.

@albertyw
Copy link
Contributor Author

@jkodumal FYI I've rebased and added 3 more commits though it shouldn't change the test status.

@jkodumal
Copy link
Contributor

Errors were fixed in #42

albertyw added a commit that referenced this pull request Nov 1, 2015
More refactoring and add tests
@albertyw albertyw merged commit 13093de into master Nov 1, 2015
@albertyw albertyw deleted the refactor branch November 1, 2015 07:26
ashanbrown pushed a commit that referenced this pull request Jan 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants