Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-41026: Path-like object support for mailbox module #20976

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

loz-hurst
Copy link

@loz-hurst loz-hurst commented Jun 19, 2020

Added support for Path-like objects (i.e. pathlib.Path) to mailbox.

https://bugs.python.org/issue41026

@loz-hurst loz-hurst requested a review from a team as a code owner June 19, 2020 11:36
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@loz-hurst

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

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

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

@loz-hurst loz-hurst changed the title dpo-41026: Path-like object support for mailbox module bpo-41026: Path-like object support for mailbox module Jun 19, 2020
Copy link
Contributor

@remilapeyre remilapeyre 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 this change @loz-hurst ! It looks good, could you add tests for this feature?

Doc/library/mailbox.rst Outdated Show resolved Hide resolved
@loz-hurst
Copy link
Author

Thanks for this change @loz-hurst ! It looks good, could you add tests for this feature?

Certainly, I'm on leave from work next week so I'll sort this out then.

I am also going to do the other half of the work and migrate the os.foo calls to use pathlib where appropriate.

@loz-hurst
Copy link
Author

I have reworked mailbox to migrate is to pathlib (where appropriate). Tests have also been migrated/updated as appropriate and additional test to verify construction with a string path (the current behaviour) correctly initialises the Path internally. New behaviour is inherently tested by the migration of the test suite to pathlib.

There's quite a few PEP8 issues with this module, I've corrected most (I need to check naming conventions) in a separate branch in my fork. I'm not sure what the policy is on this - would you accept a new issue & MR to make the module PEP8 compliant or prefer it is just left as-is? As discussed on BPO, this is a very old module and code, and I do not intend to offend anyone by offering to correct the style.

@remilapeyre
Copy link
Contributor

Hi @loz-hurst, I'm sorry but I wish you add kept the approach you took first. The goal is just to make sure that mailbox can accept a Path object as input, not to update its internal code as this makes the change much larger without benefits and it can introduce bugs.

@loz-hurst
Copy link
Author

Hi @loz-hurst, I'm sorry but I wish you add kept the approach you took first. The goal is just to make sure that mailbox can accept a Path object as input, not to update its internal code as this makes the change much larger without benefits and it can introduce bugs.

I thought it would be better to migrate to the new way of doing things, while the module was getting a bit of TLC. I presume that there will not be two ways of doing all the things that pathlib now does forever (going against PEP 20's "There should be one-- and preferably only one --obvious way to do it.") and fixing it now for the foreseeable future, as has happened in other modules looking at the change history, is surely a good thing?

In my mind this bug and change was always "update mailbox to use pathlib".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants