Skip to content

bpo-34353: Added sockets to stat.filemode fallback python implementation#8703

Merged
benjaminp merged 4 commits into
python:masterfrom
FortStatement:fix-issue-34353-master
Aug 10, 2018
Merged

bpo-34353: Added sockets to stat.filemode fallback python implementation#8703
benjaminp merged 4 commits into
python:masterfrom
FortStatement:fix-issue-34353-master

Conversation

@FortStatement

@FortStatement FortStatement commented Aug 7, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown 
[bpo-34353](https://www.bugs.python.org/issue34353): Add sockets to stat.filemode fallback python implementation

https://bugs.python.org/issue34353

@the-knights-who-say-ni

the-knights-who-say-ni commented Aug 7, 2018

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

When your account is ready, please add a comment in this pull request
and a Python core developer will remove the CLA not signed label
to make the bot check again.

You can check yourself
to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@FortStatement FortStatement changed the title Added sockets to stat.filemode fallback python implementation bpo-34353: Added sockets to stat.filemode fallback python implementation Aug 7, 2018
@FortStatement

FortStatement commented Aug 7, 2018

Copy link
Copy Markdown
Author

New account, only now signed the CLA 👍

@benjaminp

benjaminp commented Aug 8, 2018

Copy link
Copy Markdown
Contributor

Could you update Lib/test/test_stat.py to ensure this behavior is tested, too?

benjaminp
benjaminp reviewed Aug 9, 2018
View reviewed changes

@benjaminp benjaminp left a comment

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.

Looks good. Just two more nits.

Comment thread Lib/test/test_stat.py Outdated

@benjaminp benjaminp Aug 9, 2018

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.

It would cleaner to either do with socket.socket(socket.AF_UNIX): <rest of function> or use self.addCleanup(s.close) after the first line.

@FortStatement FortStatement Aug 9, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed

@FortStatement FortStatement Aug 9, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I didn't realise you could use socket as a context manager, I'll definitely be using that in the future! Thanks!

Comment thread Misc/NEWS.d/next/Core and Builtins/2018-08-08-22-28-53.bpo-34353.mijzVx.rst Outdated

@benjaminp benjaminp Aug 9, 2018

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.

You can add yourself to Misc/ACKS. Usually, we leave names off NEWS entries, though.

@FortStatement FortStatement Aug 9, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed

@FortStatement
FortStatement force-pushed the fix-issue-34353-master branch from 2875629 to cb9b170 Compare August 9, 2018 15:46
@benjaminp
benjaminp merged commit b92c526 into python:master Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjaminp benjaminp benjaminp left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@FortStatement @the-knights-who-say-ni @benjaminp @bedevere-bot