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

New function uid_expunge, which requires the capability UIDPLUS. #508

Merged
merged 4 commits into from
Aug 19, 2023
Merged

New function uid_expunge, which requires the capability UIDPLUS. #508

merged 4 commits into from
Aug 19, 2023

Conversation

axoroll7
Copy link
Contributor

The function expunge can raise an error at the server side, when the function is utilized with messages, and the server does not support the capability UIDPLUS.

Closes #496

The function `expunge` can raise an error at the server side, when the function is utilized with *messages*, and the server does not support the capability UIDPLUS.

Closes #496
@mjs
Copy link
Owner

mjs commented Aug 16, 2023

Thanks very much for this. The change looks good but could you address the formatting issue mentioned by black?

@mjs
Copy link
Owner

mjs commented Aug 16, 2023

Also, I've just fixed up the project's Github Actions config so if you rebase onto master or merge master the actions should work now.

Thanks to the linter "black", the line sizes were adjusted.
The function description was adapted to use 80 columns (PEP 8).
Context about the original ``expunge`` function were added.
@axoroll7
Copy link
Contributor Author

Hello,
Thank you for your amazing library !

I updated the pull request, to address the linter issue.
I added some context in the function description.
English is not my first language, I would like you to read carefully each word to correct me.

Comment on lines 1510 to 1513
"""Same functionality as ``expunge``, but *messages* must be
specified, and the capability UIDPLUS is tested beforehand. It should
be more fail-proof than ``expunge`` with *messages*, which cannot be
updated to prevent breaking compability with existing codebase.
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think the docstring requires quite as much detail. Something like this should do:

Expunge deleted messages with the specified message ids from the folder. This requires the UIDPLUS capability.

See :rfc:4315#section-2.1 section 2.1 for more details.

Then please extend the docstring on the expunge method to discourage use of messages there. Something like:

Use of the messages argument is discouraged. Please see the uid_expunge method instead.

I would be ok with this replacing the existing paragraph about the messages argument.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjs
Your choice of words is better than mine, I am using it without modification. New commit e0cbf56 and ea5ef55

More "formal" :
* Removed ``uid_expunge`` first appearance context.
* Deprecated notice for ``expunge`` with *messages*.
@axoroll7
Copy link
Contributor Author

@mjs
Your choice of words is better than mine, I am using it without modification. New commit e0cbf56 and ea5ef55

@axoroll7
Copy link
Contributor Author

axoroll7 commented Aug 18, 2023

The depreciation notice should be at the first line ?

@axoroll7 axoroll7 mentioned this pull request Aug 18, 2023
Copy link
Owner

@mjs mjs left a comment

Choose a reason for hiding this comment

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

This is great. Thanks very much.

@mjs mjs merged commit 458ac4d into mjs:master Aug 19, 2023
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.

expunge func assumes UIDPLUS capability and uses UID EXPUNGE in error
2 participants