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 typing to URL #1084

Merged
merged 33 commits into from
Sep 3, 2024
Merged

Add typing to URL #1084

merged 33 commits into from
Sep 3, 2024

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Sep 2, 2024

What do these changes do?

Add typing to URL

Are there changes in behavior for the user?

no runtime changes

yarl/_url.py Dismissed Show dismissed Hide dismissed
yarl/_url.py Dismissed Show dismissed Hide dismissed
yarl/_url.py Dismissed Show dismissed Hide dismissed
yarl/_url.py Dismissed Show dismissed Hide dismissed
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Sep 2, 2024
CHANGES/1084.contrib.rst Outdated Show resolved Hide resolved
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
yarl/_url.py Show resolved Hide resolved
Copy link

codecov bot commented Sep 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.88%. Comparing base (d4e7601) to head (24768b8).
Report is 318 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1084      +/-   ##
==========================================
+ Coverage   93.06%   94.88%   +1.82%     
==========================================
  Files          31       30       -1     
  Lines        4424     4358      -66     
  Branches      370      382      +12     
==========================================
+ Hits         4117     4135      +18     
+ Misses        281      197      -84     
  Partials       26       26              
Flag Coverage Δ
CI-GHA 94.83% <100.00%> (+1.82%) ⬆️
MyPy 38.91% <97.50%> (+11.94%) ⬆️
OS-Linux 99.34% <99.12%> (+<0.01%) ⬆️
OS-Windows 99.44% <99.12%> (+<0.01%) ⬆️
OS-macOS 99.02% <99.12%> (+<0.01%) ⬆️
Py-3.10.11 98.90% <98.24%> (-0.03%) ⬇️
Py-3.10.14 99.16% <98.24%> (-0.03%) ⬇️
Py-3.11.9 99.16% <98.24%> (-0.03%) ⬇️
Py-3.12.5 99.16% <98.24%> (-0.03%) ⬇️
Py-3.13.0-rc.1 99.16% <98.24%> (-0.03%) ⬇️
Py-3.8.10 98.85% <98.24%> (-0.03%) ⬇️
Py-3.8.18 99.11% <98.24%> (-0.03%) ⬇️
Py-3.9.13 98.85% <98.24%> (-0.03%) ⬇️
Py-3.9.19 99.11% <98.24%> (-0.03%) ⬇️
Py-pypy7.3.11 99.16% <98.24%> (-0.03%) ⬇️
Py-pypy7.3.16 99.19% <98.24%> (-0.03%) ⬇️
VM-macos-latest 99.02% <99.12%> (+<0.01%) ⬆️
VM-ubuntu-latest 99.34% <99.12%> (+<0.01%) ⬆️
VM-windows-latest 99.44% <99.12%> (+<0.01%) ⬆️
pytest 99.34% <99.12%> (+<0.01%) ⬆️

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.

.coveragerc Outdated Show resolved Hide resolved
.coveragerc Outdated Show resolved Hide resolved
.coveragerc Outdated Show resolved Hide resolved
.coveragerc Outdated Show resolved Hide resolved
.coveragerc Outdated Show resolved Hide resolved
webknjaz and others added 4 commits September 3, 2024 02:48
yarl/_helpers_c.pyi Outdated Show resolved Hide resolved
.coveragerc Outdated Show resolved Hide resolved
.coveragerc Outdated Show resolved Hide resolved
@bdraco
Copy link
Member Author

bdraco commented Sep 3, 2024

@bdraco bdraco marked this pull request as ready for review September 3, 2024 05:54
@webknjaz
Copy link
Member

webknjaz commented Sep 3, 2024

path shows 100% diff hit on codecov but is reporting at 99.13% here...

Yeah, I suppose this was due to the proportion being different because the overall number of lines changed. I'm not seeing 99.13% there, though. Perhaps, it processed more reports after your comment.
For me, it shows 94.94% for the entire pytest flag and 99.88% for tests/ flagged with pytest and 98.15% for yarl/+pytest.

I suppose, it's okay to raise the codecov/project/lib expectation to 98.1% (leaving that 0.05% margin for future line count changes, until we can go for 100%).

yarl/_helpers_c.pyi Outdated Show resolved Hide resolved
yarl/_url.py Show resolved Hide resolved
yarl/_url.py Outdated Show resolved Hide resolved
yarl/_url.py Outdated Show resolved Hide resolved
yarl/_url.py Outdated Show resolved Hide resolved
@bdraco bdraco merged commit e70f9d4 into master Sep 3, 2024
47 of 49 checks passed
@bdraco bdraco deleted the typing branch September 3, 2024 19:40
@bdraco
Copy link
Member Author

bdraco commented Sep 3, 2024

I'm going to do a RC0 release so I can run this in the CI of a few projects since typing changes are always hard to predict the impact

@sergeniushpe
Copy link

sergeniushpe commented Sep 5, 2024

Hello, I think this change broke python 3.8 support:

../lib/python3.8/site-packages/yarl/_url.py:606: in URL
    def query(self) -> MultiDictProxy[str]:
E   TypeError: 'type' object is not subscriptable

If I downgrade to release 1.9.7 it works again.

You might want to update python requirements to indicate python>=3.9 is required

@bdraco
Copy link
Member Author

bdraco commented Sep 5, 2024

Hello, I think this change broke python 3.8 support:

../lib/python3.8/site-packages/yarl/_url.py:606: in URL
    def query(self) -> MultiDictProxy[str]:
E   TypeError: 'type' object is not subscriptable

If I downgrade to release 1.9.7 it works again.

You might want to update python requirements to indicate python>=3.9 is required

Would you please open an issue report, I will fix it shortly

@bdraco
Copy link
Member Author

bdraco commented Sep 5, 2024

Please include the multi dict version you are using in the issue

@Dreamsorcerer
Copy link
Member

Please include the multi dict version you are using in the issue

As far as I can see, multidict has supported that syntax for ~6 years...
But, maybe we should increase the minimum version of multidict that yarl supports.

@bdraco
Copy link
Member Author

bdraco commented Sep 5, 2024

I had to take the python 3.8 install back to multidict 4 to get it to fail. There might be a newer version that fails though as I stopped trying them once I confirmed the issue. Also even with 4 it didn’t fail on python 3.9 so I suspect the issue will go away on its own soon when we drop 3.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants