Skip to content

bpo-35942: Improve the error message if __fspath__ returns invalid types in path_converter#11831

Merged
pablogsal merged 4 commits intopython:masterfrom
pablogsal:bpo35942
Feb 18, 2019
Merged

bpo-35942: Improve the error message if __fspath__ returns invalid types in path_converter#11831
pablogsal merged 4 commits intopython:masterfrom
pablogsal:bpo35942

Conversation

@pablogsal
Copy link
Copy Markdown
Member

@pablogsal pablogsal commented Feb 12, 2019

@ZackerySpytz
Copy link
Copy Markdown
Contributor

The issue number is wrong.

Comment thread Modules/posixmodule.c Outdated
@pablogsal pablogsal force-pushed the bpo35942 branch 2 times, most recently from 11b6b0b to f663492 Compare February 12, 2019 22:40
@pablogsal pablogsal changed the title bpo-36942: Delegate on PyOS_FSPath when checking for __fspath__ in path_converter bpo-35942: Delegate on PyOS_FSPath when checking for __fspath__ in path_converter Feb 12, 2019
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

I don't like this. It makes error messages for incorrect type of arguments worse.

Change only error messages for __fspath__() returning incorrect result.

@bedevere-bot
Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@pablogsal
Copy link
Copy Markdown
Member Author

@serhiy-storchaka I have changed the approach to only correct messages for __fspath__() returning an incorrect result.

I have made the requested changes; please review again

@bedevere-bot
Copy link
Copy Markdown

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

@pablogsal pablogsal changed the title bpo-35942: Delegate on PyOS_FSPath when checking for __fspath__ in path_converter bpo-35942: Improve the error message if __fspath__ returns invalid types in path_converter Feb 17, 2019
Comment thread Modules/posixmodule.c Outdated
Comment thread Modules/posixmodule.c Outdated
Comment thread Modules/posixmodule.c Outdated
Comment thread Modules/posixmodule.c Outdated
@pablogsal
Copy link
Copy Markdown
Member Author

@serhiy-storchaka Thank you very much for the thorough review! I have addressed your feedback in 477daf8.

@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error and removed skip news labels Feb 18, 2019
@serhiy-storchaka
Copy link
Copy Markdown
Member

serhiy-storchaka commented Feb 18, 2019

I think this bugfix is worth a news entry.

Copy link
Copy Markdown
Contributor

@ambv ambv left a comment

Choose a reason for hiding this comment

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

Thanks for jumping on this so quickly, @pablogsal!

Comment thread Modules/posixmodule.c
}

/* still owns a reference to the original object */
Py_DECREF(o);
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.

A little question, Why don't make Py_DECREF(res); too?

@pablogsal pablogsal merged commit 09fbcd6 into python:master Feb 18, 2019
@miss-islington
Copy link
Copy Markdown
Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@pablogsal pablogsal deleted the bpo35942 branch February 18, 2019 10:46
@bedevere-bot
Copy link
Copy Markdown

GH-11912 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 18, 2019
…pes in path_converter (pythonGH-11831)

The error message emitted when returning invalid types from __fspath__ in interfaces that allow passing PathLike objects has been improved and now it does explain the origin of the error.
(cherry picked from commit 09fbcd6)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
miss-islington added a commit that referenced this pull request Feb 18, 2019
…pes in path_converter (GH-11831)

The error message emitted when returning invalid types from __fspath__ in interfaces that allow passing PathLike objects has been improved and now it does explain the origin of the error.
(cherry picked from commit 09fbcd6)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants