Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry GET calls within _request method 5 times before giving up, and fix coverage #120

Closed
goessebr opened this issue Dec 15, 2021 · 3 comments
Assignees
Labels
Milestone

Comments

@goessebr
Copy link
Contributor

The coverage tests on the master branch are broken.
image

The code in

if not res.encoding:
res.encoding = 'utf-8'
is probably obsolete. A response object seems to get a default encoding of utf-8 if no charset is given.
(noticed in #119 (comment))

This code was probably obsolete before but coverall seems to have a problem with it now due the algoritm used to calculate the coverage.

The obsolete code can be removed, I'm not not sure about the test. I might be safe to keep it.

@koenedaele @Wim-De-Clercq your opinions?

@goessebr goessebr added this to the Sprint 170 milestone Dec 15, 2021
@Wim-De-Clercq
Copy link
Contributor

The coverage check is only broken for this 1 commit, next commit would be fine. This can be safely ignored.

As for the encoding, a JSON response does not receive a charset in the header because it is technically illegal for application/json to have one.

Setting the encoding should be unnecessary. See more at Pylons/pyramid#2860 and the links there.
tldr; it would be illegal to encode a json response in something else than utf-8, 16 or 32.

And the requests library knows this

        if not self.encoding and self.content and len(self.content) > 3:
            # No encoding set. JSON RFC 4627 section 3 states we should expect
            # UTF-8, -16 or -32. Detect which one to use; If the detection or
            # decoding fails, fall back to `self.text` (using charset_normalizer to make
            # a best guess).

The encoding parameter we set would give a slight speedboost because requests doesn't have to go decide between utf 8, 16, or 32. But it's probably total overkill for us to do so.

I'm all for removing it.

@koenedaele
Copy link
Member

Looking at that code, in other projects we retry requests a few times before giving up. Would it be useful to do the same here?

@Wim-De-Clercq
Copy link
Contributor

Yea I don't see why not.

@Wim-De-Clercq Wim-De-Clercq self-assigned this Dec 16, 2021
@goessebr goessebr changed the title Remove obsolute if-statement with check if encoding is present Retry request 5 times before giving up, and fix coverage Dec 17, 2021
@goessebr goessebr changed the title Retry request 5 times before giving up, and fix coverage Retry GET calls within _request method 5 times before giving up, and fix coverage Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants