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

Replace deprecated multipart argument memfile_limit with spool_limit #79

Merged
merged 3 commits into from
Nov 4, 2024

Conversation

stollero
Copy link
Contributor

@stollero stollero commented Oct 2, 2024

Hello everyone,

memfile_limit has been replaced by spool_limit, see https://github.com/defnull/multipart/blob/master/multipart.py#L576.

Unfortunately they use self.spool_limit = memfile_limit or spool_limit, so if memfile_limit = 0 it will always be the default spool_limit instead.

@icemac icemac requested a review from mgedmin October 23, 2024 06:25
Copy link
Member

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

Looks good if we make sure setup.py requires multipart >= the version that added the spool_limit argument. What version was it, by the way?

@mgedmin
Copy link
Member

mgedmin commented Oct 23, 2024

(You'll want to rebase on git master to fix the CI failure.)

@mgedmin
Copy link
Member

mgedmin commented Oct 23, 2024

I found a button on GitHub that allowed me to rebase your branch. Hope I didn't step on any toes while doing it!

@stollero
Copy link
Contributor Author

Thanks for the review. I will update the setup.py 👍

Looks good if we make sure setup.py requires multipart >= the version that added the spool_limit argument. What version was it, by the way?

The bump to multipart v1.0.0

@icemac
Copy link
Member

icemac commented Oct 29, 2024

Thank you for your contribution.

According to the contributing policies of the zopefoundation organization you need to sign a contributor agreement before any non-trivial change can be merged. For details please consult the Contributing guidelines for zopefoundation projects.

Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

I like the changes, additionally I'd like to request an entry in the change log.

@stollero
Copy link
Contributor Author

@icemac Updated changelog and signed agreement 👍

@icemac icemac merged commit 76f741c into zopefoundation:master Nov 4, 2024
11 checks passed
@icemac
Copy link
Member

icemac commented Nov 4, 2024

Thank you for this PR. 😃 And welcome onboard!

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

Successfully merging this pull request may close these issues.

3 participants