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

add keyword argument encode_url for ClientSession._request #8466

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

RomySaputraSihananda
Copy link

Hello Dev Team,

I have implemented a new feature enhancement in the ClientSession._fetch method. This enhancement introduces an optional parameter named encode_url, which allows users to control whether the URL should be encoded before being used in the HTTP request. Below are the technical details:

  1. Changes: Added the optional parameter encode_url to the ClientSession._fetch method.
  2. Implementation: I added conditional logic where the URL encoding is performed based on the value of encode_url.
  3. Testing: I have included unit tests to ensure that this feature works as expected in various scenarios.
    I believe that this feature will enhance the flexibility and usability of aiohttp in cases where direct control over URL encoding is necessary. I am open to addressing any feedback or changes required before merging.

Thank you for your time and consideration.

Best regards,

Copy link

codecov bot commented Jun 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.57%. Comparing base (fc32f69) to head (b85d50e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8466      +/-   ##
==========================================
- Coverage   97.64%   97.57%   -0.07%     
==========================================
  Files         107      107              
  Lines       33067    33067              
  Branches     3885     3885              
==========================================
- Hits        32288    32266      -22     
- Misses        562      580      +18     
- Partials      217      221       +4     
Flag Coverage Δ
CI-GHA 97.50% <100.00%> (-0.06%) ⬇️
OS-Linux 97.16% <100.00%> (-0.06%) ⬇️
OS-Windows 95.64% <ø> (ø)
OS-macOS 96.88% <100.00%> (ø)
Py-3.10.11 97.03% <100.00%> (ø)
Py-3.10.14 96.98% <100.00%> (ø)
Py-3.11.9 97.20% <100.00%> (ø)
Py-3.12.3 95.48% <ø> (-1.86%) ⬇️
Py-3.12.4 96.99% <100.00%> (ø)
Py-3.8.10 95.41% <ø> (ø)
Py-3.8.18 96.87% <100.00%> (ø)
Py-3.9.13 97.02% <100.00%> (+<0.01%) ⬆️
Py-3.9.19 96.97% <100.00%> (ø)
Py-pypy7.3.16 ?
VM-macos 96.88% <100.00%> (ø)
VM-ubuntu 97.16% <100.00%> (-0.06%) ⬇️
VM-windows 95.64% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Dreamsorcerer
Copy link
Member

I think this is just duplicating the URL object, so not sure it makes much difference to the user:

.get(url, encode_url=False) vs .get(URL(url, encoded=True))

We get enough bug reports from users that I'm thinking we should change the default in future though. @webknjaz What do you think about disabling the encoding option by default in v4?

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.

2 participants