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.

Sign up for GitHub

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

Jump to bottom

Allow int Subclasses in Query #492

Closed
wants to merge 7 commits into from
Closed

Allow int Subclasses in Query #492

wants to merge 7 commits into from

Conversation

Exahilosys
Copy link

@Exahilosys Exahilosys commented Aug 8, 2020 •
edited
Loading

I have found myself having to do explicit type conversions of int subclasses that should definitely work.

What do these changes do?

This new check allows all int subclasses through except bool, which should obviously be rejected.

Are there changes in behavior for the user?

Nope. I can't imagine backward compatibility being an issue.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

@Exahilosys
Allow int Subclasses in Query
eaddc6f 
I have found myself having to do explicit type conversions of `int` subclasses that should definitely work. 
This new check allows all `int` subclasses through except `bool`, which should obviously be rejected.
@codecov
Copy link

codecov bot commented Aug 8, 2020 •
edited
Loading

Codecov Report

Merging #492 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #492   +/-   ##
=======================================
  Coverage   99.57%   99.57%           
=======================================
  Files           4        4           
  Lines         708      712    +4     
  Branches      157      158    +1     
=======================================
+ Hits          705      709    +4     
  Misses          3        3
Impacted Files Coverage Δ
yarl/_url.py 99.44% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa14d56...f1038da. Read the comment docs.

webknjaz
webknjaz reviewed Aug 9, 2020
View reviewed changes
yarl/_url.py Outdated
@@ -904,7 +904,7 @@ def _query_seq_pairs(cls, quoter, pairs):
def _query_var(v):
if isinstance(v, str):
return v
if type(v) is int: # no subclasses like bool
if isinstance(v, int) and not isinstance(v, bool): # no subclasses like bool
Copy link
Member

Choose a reason for hiding this comment

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

This will probably make it slower so I'll leave it to @asvetlov to decide. But you'd probably have to add tests for this (both negative and positive) and find other places in code that use the same idiom.

Also, what if such int subclass implements a weird __str__() method that returns something odd? This would create hard-to-debug issues in the calling code, especially if your colleagues don't know YARL internals. Is it really worth it? This could probably be mitigated by an extra check for v.__str__ is int.__str__ but isn't it overkill?

@asvetlov
Copy link
Member

Would you describe your use case?

@Exahilosys
Copy link
Author

@webknjaz I can add tests, please allow for some time as I'm currently on holiday. Also, instead of using the built-in str for conversion, int.__str__ could directly be used (no need to compare __str__s then). This does't feel overkill and makes conversion more consistent across the board.

@asvetlov I am making a package with a subclass of int meant to represent ids following twitter's snowflake standard with custom methods for extracting information like worker ids and timestamps. Those ids point to API resources and are sometimes used in the query part of requests.

Since aiohttp uses yarl under the hood, explicit conversions from that int subclass to str or int are required every time. Situations like this are what this PR is trying to address.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Aug 17, 2020
@Exahilosys
Copy link
Author

I have created both positive and negative tests by restructuring and expanding the current type-checking strategy to a more compact one. As an extra to the PR, I would also suggest allowing float subclasses.

@serhiy-storchaka
Copy link
Contributor

The most common int subclasses are bool and IntEnum. bool is not allowed in query because of ambiguity of its representation: 1/0, true/false, True/False, yes/no, on/off, etc. But IntEnum also overrides __str__, so there is ambiguity of its representation: as MsgFlag.MSG_CONFIRM or 2048? And the representation of standard enums may be changed in Python 3.10, so it will be socket.MSG_CONFIRM.

@asvetlov asvetlov mentioned this pull request Sep 23, 2020
Merged
@asvetlov
Copy link
Member

Fixed by #505

@asvetlov asvetlov closed this Sep 23, 2020
@asvetlov
Copy link
Member

@Exahilosys thanks for the initial PR!
I've slightly improved it and merged.

netbsd-srcmastr referenced this pull request in NetBSD/pkgsrc Oct 2, 2020
@0-wiz-0
py-yarl: update to 1.6.0.
a5b6ae0 
Fix dependencies.

1.6.0 (2020-09-23)
==================

Features
--------

- Allow for int and float subclasses in query, while still denying bool.
  `#492 <https://github.com/aio-libs/yarl/issues/492>`_


Bugfixes
--------

- Do not requote arguments in ``URL.build()``, ``with_xxx()`` and in ``/`` operator.
  `#502 <https://github.com/aio-libs/yarl/issues/502>`_
- Keep IPv6 brackets in ``origin()``.
  `#504 <https://github.com/aio-libs/yarl/issues/504>`_
bmwiedemann referenced this pull request in bmwiedemann/openSUSE Oct 29, 2020
@bmwiedemann
Update python-yarl to version 1.6.0 / rev 14 via SR 838272
f74b93d 
https://build.opensuse.org/request/show/838272
by user dirkmueller + dimstar_suse
- update to 1.6.0:
  - Allow for int and float subclasses in query, while still denying bool.
    `#492 <https://github.com/aio-libs/yarl/issues/492>`_
  - Do not requote arguments in ``URL.build()``, ``with_xxx()`` and in ``/`` operator.
    `#502 <https://github.com/aio-libs/yarl/issues/502>`_
  - Keep IPv6 brackets in ``origin()``.
    `#504 <https://github.com/aio-libs/yarl/issues/504>`_
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webknjaz webknjaz webknjaz left review comments

Assignees
No one assigned
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Exahilosys @asvetlov @serhiy-storchaka @webknjaz