-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
base: main
Are you sure you want to change the base?
Conversation
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 MissingOur records indicate the following people have not signed the CLA: 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 You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
There was a problem hiding this 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?
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. |
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. |
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 |
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". |
Added support for Path-like objects (i.e. pathlib.Path) to mailbox.
https://bugs.python.org/issue41026