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

bpo-30732: json.dumps() lacks information about RecursionError relate… by soolabettu · Pull Request #2857 · python/cpython · GitHub

/ cpython Public

bpo-30732: json.dumps() lacks information about RecursionError relate… - #2857

Closed
soolabettu wants to merge 4 commits into
python:mainfrom
soolabettu:bpo-30732
ConversationCommits (4)ChecksFiles changed
Closed

bpo-30732: json.dumps() lacks information about RecursionError relate…#2857
soolabettu wants to merge 4 commits into
python:mainfrom
soolabettu:bpo-30732

Conversation

soolabettu commented Jul 25, 2017 •
edited by serhiy-storchaka
Loading

Copy link
Copy Markdown
Contributor

mlouielu reviewed Jul 25, 2017
View reviewed changes
Comment thread Modules/_json.c Outdated
}
}
newobj = PyObject_CallFunctionObjArgs(s->defaultfn, obj, NULL);
if (obj != NULL && newobj != NULL && newobj->ob_type == obj->ob_type) {

mlouielu Jul 25, 2017

Copy link
Copy Markdown
Contributor

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

How about if (PyObject_IsInstance(newobj, (PyObject *) Py_TYPE(obj))) { ?

Also, I think this can put above the check of if (newobj == NULL)

soolabettu Jul 30, 2017

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

Wouldn't PyObject_IsInstance raise false positives for our condition? For this fix, we would want the exact types to match, isn't it?

serhiy-storchaka reviewed Jul 31, 2017
View reviewed changes

serhiy-storchaka 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

I'm not sure this is the right solution. Here are just style comments.

Comment thread Modules/_json.c Outdated
Comment thread Modules/_json.c Outdated

soolabettu commented Aug 1, 2017

Copy link
Copy Markdown
Contributor Author

Ignore the identation changes please, its still not aligned.

github-actions Bot commented Feb 20, 2022

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Feb 20, 2022
KrzysztofNazarewski mannequin mentioned this pull request Apr 10, 2022
Closed
github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jul 29, 2022

github-actions Bot commented Aug 29, 2022

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Aug 29, 2022

serhiy-storchaka commented Dec 24, 2023

Copy link
Copy Markdown
Member

I was not able to modify the original PR, so I created a new one: #113458.

serhiy-storchaka mentioned this pull request Dec 24, 2023
Closed
github-actions Bot removed the stale Stale PR or inactive for long period of time. label Dec 25, 2023

github-actions Bot commented Jan 25, 2024

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Jan 25, 2024

encukou commented Jan 3, 2025

Copy link
Copy Markdown
Member

Fixed in #122165 instead.

encukou closed this Jan 3, 2025
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

serhiy-storchaka serhiy-storchaka left review comments
+1 more reviewer
mlouielu mlouielu left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting review stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL