Skip to content
This repository has been archived by the owner on Sep 23, 2024. It is now read-only.

Simplify response code check #147

Closed
wants to merge 1 commit into from

Conversation

major
Copy link
Contributor

@major major commented May 21, 2018

The response object has an ok method that returns True if the status
code is < 400.

This works towards #3.

Signed-off-by: Major Hayden [email protected]

The response object has an `ok` method that returns True if the status
code is < 400.

Signed-off-by: Major Hayden <[email protected]>
@major major added the enhancement New feature or request label May 21, 2018
@major major self-assigned this May 21, 2018
@coveralls
Copy link

Pull Request Test Coverage Report for Build 471

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 46.658%

Changes Missing Coverage Covered Lines Changed/Added Lines %
skt/init.py 0 1 0.0%
Totals Coverage Status
Change from base Build 463: 0.0%
Covered Lines: 684
Relevant Lines: 1466

💛 - Coveralls

@veruu
Copy link
Contributor

veruu commented May 21, 2018

While technically this should be ok (no pun intended), we are retrieving directly the mbox and no redirects should occur. If they do, there's a chance something is wrong, and this would hide it.

@major
Copy link
Contributor Author

major commented May 22, 2018

@veruu True, but the requests module automatically follows redirects, so this should be okay as long as the resulting redirect returns something < 400.

http://docs.python-requests.org/en/master/api/

@veruu
Copy link
Contributor

veruu commented May 22, 2018

@veruu True, but the requests module automatically follows redirects,

That's exactly my point. How can we just roll with the redirects when we don't know where they end up, as they shouldn't be there in the first place? We'd end up with something that's not mbox and get unrelated errors later. I was thinking about using raise_for_status which basically does the same thing you propose (just more simple since we don't need to throw the exception ourselves but opted not to for exactly this reason.

@major
Copy link
Contributor Author

major commented May 22, 2018

I see your point. We can forget about this for now.

@major major closed this May 22, 2018
@major major deleted the simplify-response-code-check branch May 22, 2018 12:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants