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

🐛 Fix encoding data with None values and more #119

Merged
merged 6 commits into from
May 6, 2024

Conversation

karosc
Copy link

@karosc karosc commented May 3, 2024

This addresses #118, inspired by the code in requests.

Essentially this PR will allow encoding any non-iterable object as string. This is a general solution to the specific problem in #118. Since None is not iterable, it is encoded "None". Similarly, any non-iterable object will be encoded as str(obj).

@karosc karosc changed the title fix encoding data with None values and more 🐛 fix encoding data with None values and more May 3, 2024
@karosc karosc changed the title 🐛 fix encoding data with None values and more 🐛 Fix encoding data with None values and more May 3, 2024
Copy link

@Ousret Ousret left a comment

Choose a reason for hiding this comment

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

the test only make sure the error does not occurs. we should verify that the "None" values are excluded.

tests/test_requests.py Outdated Show resolved Hide resolved
tests/test_requests.py Outdated Show resolved Hide resolved
@Ousret Ousret linked an issue May 4, 2024 that may be closed by this pull request
@karosc
Copy link
Author

karosc commented May 4, 2024

@Ousret, you're totally right. I had how requests handles the None values totally wrong. I updated the PR and unit tests to check the values of the encoding as you suggested. I also wrote a short script to test that requests and niquests produce the same results when posting various python types:

import niquests
import requests

class Class:
        pass

class ClassStringable:
    def __str__(self):
        return "🔥arbClassStringable🔥"

url = "https://httpbin.org/post"
body = {
    "string": "string",
    "float": 0.01,
    "int": 100,
    "bool": True,
    "None": None,
    "plain_class": Class(),
    "stringable_class": ClassStringable(),
}

resp_re = requests.post(url, data = body)
resp_ni = niquests.post(url, data = body)


for key in ['args', 'data','files','form','json']:
    try:
        assert resp_re.json()[key] == resp_ni.json()[key]
    except AssertionError:
        print(resp_re.json()[key],'\n\n\n',resp_ni.json()[key])

Copy link

@Ousret Ousret left a comment

Choose a reason for hiding this comment

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

Almost there.

tests/test_requests.py Outdated Show resolved Hide resolved
src/niquests/models.py Outdated Show resolved Hide resolved
tests/test_requests.py Outdated Show resolved Hide resolved
tests/test_requests.py Show resolved Hide resolved
tests/test_requests.py Show resolved Hide resolved
tests/test_requests.py Outdated Show resolved Hide resolved
@Ousret Ousret merged commit 0b7fa53 into jawah:main May 6, 2024
29 of 31 checks passed
Ousret added a commit that referenced this pull request May 6, 2024
**Fixed**
- Fixed encoding data with None values and other objects. This was a regression introduced in our v3. #119

**Changed**
- Various minor performance improvements.
@Ousret Ousret mentioned this pull request May 6, 2024
Ousret added a commit that referenced this pull request May 6, 2024
**Fixed**
- Fixed encoding data with None values and other objects. This was a
regression introduced in our v3. #119

**Changed**
- Various minor performance improvements.
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.

Encoding None Values
2 participants