FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix/1920 by Mr-Sunglasses · Pull Request #1921 · dry-python/returns · GitHub

/ returns Public

Fix/1920 - #1921

Open
Mr-Sunglasses wants to merge 2 commits into
dry-python:masterfrom
Mr-Sunglasses:fix/1920
ConversationCommits (2)ChecksFiles changed
Open

Fix/1920#1921
Mr-Sunglasses wants to merge 2 commits into
dry-python:masterfrom
Mr-Sunglasses:fix/1920

Conversation

Mr-Sunglasses commented Aug 29, 2024 •
edited
Loading

Copy link
Copy Markdown

I have made things!

Checklist

  • I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
  • I have created at least one test case for the changes I have made
  • I have updated the documentation for the changes I have made
  • I have added my changes to the CHANGELOG.md

Related issues

fix: #1920

🙏 Please, if you or your company finds dry-python valuable, help us sustain the project by sponsoring it transparently on https://github.com/sponsors/dry-python. As a thank you, your profile/company logo will be added to our main README which receives hundreds of unique visitors per day.

Mr-Sunglasses commented Aug 29, 2024

Copy link
Copy Markdown
Author

I have made things!

Checklist

* [x]  I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)

* [ ]  I have created at least one test case for the changes I have made

* [ ]  I have updated the documentation for the changes I have made

* [ ]  I have added my changes to the `CHANGELOG.md`

Related issues

fix: #1920

🙏 Please, if you or your company finds dry-python valuable, help us sustain the project by sponsoring it transparently on https://github.com/sponsors/dry-python. As a thank you, your profile/company logo will be added to our main README which receives hundreds of unique visitors per day.

cc. @sobolevn

sobolevn reviewed Aug 30, 2024
View reviewed changes

sobolevn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please, add tests cases for:

  • Empty replace
  • Replace with correct value
  • Replace with incorrect values

Comment thread returns/primitives/container.py

def __replace__(self, **changes: Any) -> 'BaseContainer':
"""Create a new instance with specified changes."""
if set(changes.keys()) - set(self.__slots__):

sobolevn Aug 30, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think that we can simplify this to only allow _inner_value change. Nothing else.

Comment thread returns/primitives/container.py
# backward compatibility with 0.19.0 and earlier
object.__setattr__(self, '_inner_value', state) # noqa: WPS609

def __replace__(self, **changes: Any) -> 'BaseContainer':

sobolevn Aug 30, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok, I was wrong about adding this method to BaseContainer base class. I think that we need to add this to Unwrappable (?) instead. Basically, right now - this is not type-safe at all.

And add a proper type annotations. And add type tests for it.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

sobolevn sobolevn left review comments

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support copy.replace from 3.13

2 participants

Back | FazBrowse Home | New Git URL