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

Add BaseException.__reduce__ by fanninpm · Pull Request #3665 · RustPython/RustPython · GitHub

Repository navigation

Add BaseException.__reduce__ - #3665

Merged
youknowone merged 6 commits into
RustPython:mainfrom
fanninpm:baseexception-reduce
Apr 29, 2022
Merged

youknowone merged 6 commits into
RustPython:mainfrom
fanninpm:baseexception-reduce

Conversation

Copy link
Copy Markdown
Contributor

Addresses three items in #3611.

cc @Snowapril

Copy link
Copy Markdown
Contributor

Looks good! This PR can be a good specimen for the other remaining works. Does "three items" mean OSError, ImportError, BaseException?

Copy link
Copy Markdown
Contributor Author

Does "three items" mean OSError, ImportError, BaseException?

Yes. I only had to add things to BaseException, since ImportError and OSError inherit from BaseException.

Comment thread vm/src/exceptions.rs Outdated

Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Contributor Author

Unfortunately, OSError's and ImportError's reduce act different than BaseException's 😭

I would direct future contributors to look at the extend_exception! macro.

Copy link
Copy Markdown
Contributor

Ok good 😊. Except for the issue about the empty set that I commented on, everything seems fine.

youknowone 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

great, thank you! @Snowapril and thank you for the review!

Comment thread vm/src/exceptions.rs
}

#[pymethod(magic)]
fn reduce(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyTupleRef {

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

If it doesn't require type checking, this may be:

Suggested change
fn reduce(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyTupleRef {
fn reduce(zelf: PyObjectRef, vm: &VirtualMachine) -> PyTupleRef {

fanninpm Apr 29, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

It doesn't work in practice because rustc will complain:

no method named `args` found for struct `core::PyObjectRef` in the current scope
method not found in `core::PyObjectRef`

fanninpm requested a review from youknowone April 29, 2022 14:59
youknowone merged commit 4c39668 into RustPython:main Apr 29, 2022
fanninpm deleted the baseexception-reduce branch April 29, 2022 22:41
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
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.

3 participants


Back | FazBrowse Home | New Git URL