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

Remove default empty list #92

Merged
merged 5 commits into from Oct 30, 2021

Conversation

azriel1rf
Copy link
Contributor

@azriel1rf azriel1rf commented Oct 28, 2021

Replace [] and {} with None in default values

Mutable variables such as [] and {} in default values may have some side effects and cause unintended results.

In the second section of 8.7 in the Python Language Reference, it is said that use None as the default.

when a default parameter value is a mutable object, such as a list or a dictionary: if the function modifies 
the object (e.g. by appending an item to a list), the default parameter value is in effect modified. This is 
generally not what was intended. A way around this is to use None as the default

https://docs.python.org/3/reference/compound_stmts.html#function-definitions

An easier explanation is in the following page:
https://nikos7am.com/posts/mutable-default-arguments/

With replacing empty collections with None, those typing are modified to Optional.

Replace Mapping with Dict Dict with Mapping

There are some typing incompatibility between Mapping and Dict. mypy reports those errors.
Actually, Mapping is different from Dict. However, assuming the usage in the tests, the suitable type is Dict Mapping.

Typing generics with standard collections (PEP 585)

Since the Python 3.9, the standard collections tuple, list and dict can be used as generics instead of Tuple, List and Dict and PEP 585 aims to remove these typing modules.
Although the supporting versions include 3.7~3.8, those variables are used as the specific standard collections rather than generics so that this modification won't have side effects also for those versions.
https://www.python.org/dev/peps/pep-0585/

  • pytest (poetry run pytest)
    • no effects. 70 passed, 9 warnings
  • mypy (poetry run mypy .)
    • 127 errors -> 122 errors with mypy 0.910

@azriel1rf
Copy link
Contributor Author

I'll modify it

@MtkN1 MtkN1 self-requested a review October 29, 2021 02:03
Copy link
Member

@MtkN1 MtkN1 left a comment

Choose a reason for hiding this comment

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

Thank you for your pull request.

Replace [] and {} with None in default values

LGTM🙇‍♂️

Replace Mapping with Dict

This is aiohttp specification.

https://github.com/aio-libs/aiohttp/blob/750b15f89c26d6a3332ac29d236764ef71e50994/aiohttp/client.py#L312

It works in Mapping format instead of Dict. I don't think there is any need to change it.

import asyncio
import pybotters

async def main():
    async with pybotters.Client() as client:
        async with client.get(
            "https://httpbin.org/get",
            # params={"foo": "bar"},
            params=[("foo", "bar")],
        ) as resp:
            data = await resp.text()
        print(data)

asyncio.run(main())

Typing generics with standard collections (PEP 585)

Python 3.7 ~ 3.8 requires from __future__ import annotations, so pytest is failing.

https://www.python.org/dev/peps/pep-0585/#implementation

I want to use the typing module according to the aiohttp source code, should this use the standard collection?

@azriel1rf
Copy link
Contributor Author

azriel1rf commented Oct 29, 2021

Thanks for the review.

  • Replace Mapping with Dict
    That's inverted. One of the Dicts is replaced with Mapping

    Finally it comes out:

    • Mapping: nothing to change
    • Dict:
      • params in client.py -> Mapping
      • the others -> dict
  • Generics
    Thanks. I use it.


Plus, I rebased commits and unintentionally be tracked authorship for previous commits(not related with me). Squash my commits at your ease when merging.

@azriel1rf azriel1rf force-pushed the remove_default_empty_list branch 2 times, most recently from 43220f6 to 6eae0b2 Compare October 29, 2021 03:31
@azriel1rf azriel1rf requested a review from MtkN1 October 29, 2021 03:44
pybotters/ws.py Outdated Show resolved Hide resolved
@MtkN1
Copy link
Member

MtkN1 commented Oct 29, 2021

** Please change the target branch to develop **

@azriel1rf azriel1rf force-pushed the remove_default_empty_list branch 2 times, most recently from e42c9f1 to b5c7c86 Compare October 29, 2021 15:19
@azriel1rf azriel1rf changed the base branch from main to develop October 29, 2021 15:37
@azriel1rf azriel1rf force-pushed the remove_default_empty_list branch 2 times, most recently from f03543c to 6eaa4d1 Compare October 29, 2021 15:57
@azriel1rf azriel1rf marked this pull request as draft October 29, 2021 16:00
@azriel1rf azriel1rf force-pushed the remove_default_empty_list branch 3 times, most recently from 6503d63 to 248bd52 Compare October 29, 2021 16:20
@azriel1rf azriel1rf marked this pull request as ready for review October 29, 2021 16:52
@azriel1rf azriel1rf requested a review from MtkN1 October 29, 2021 16:52
Copy link
Member

@MtkN1 MtkN1 left a comment

Choose a reason for hiding this comment

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

LGTM🥳
Thanks for the contribution!

@MtkN1 MtkN1 merged commit 195d39f into pybotters:develop Oct 30, 2021
@azriel1rf azriel1rf deleted the remove_default_empty_list branch October 30, 2021 09:40
@MtkN1 MtkN1 mentioned this pull request Nov 11, 2021
MtkN1 added a commit that referenced this pull request Nov 11, 2021
✨v0.8.0リリース

## Issues
✅ Bybitの型問題の抜本的な解決方法について #82
✅ バイナリのWebSocketデータのハンドリングに対応する #87
✅ GMOコインのデータストアにタイムスタンプを追加する #88
## Pull requests
✅ Remove default empty list #92
✅ add API Reference #93
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.

None yet

2 participants