KEMBAR78
bpo-46267: Correctly use compresslevel in gzip.compress by rhpvorderman · Pull Request #30416 · python/cpython · GitHub
Skip to content

Conversation

@rhpvorderman
Copy link
Contributor

@rhpvorderman rhpvorderman commented Jan 5, 2022

This is a small one-liner bug fix. I am the one responsible for introducing the bug in the first place, so I am happy I found it before 3.11 shipped.

https://bugs.python.org/issue46267

#90425

@rhpvorderman
Copy link
Contributor Author

No news item is required. This new method for gzip.compress was introduced in 3.11.

@AlexWaygood AlexWaygood added skip news type-bug An unexpected behavior, bug, or error labels Jan 5, 2022
@akulakov
Copy link
Contributor

This does require a news entry if the minor version is different vs. where bug was introduced. This is true for all bugfixes.

@rhpvorderman
Copy link
Contributor Author

This does require a news entry if the minor version is different vs. where bug was introduced. This is true for all bugfixes.

This bug was introduced in 3.11. I am hoping this will still get into main before python 3.11 releases. So no news item is required.

@akulakov
Copy link
Contributor

This does require a news entry if the minor version is different vs. where bug was introduced. This is true for all bugfixes.

This bug was introduced in 3.11. I am hoping this will still get into main before python 3.11 releases. So no news item is required.

3.11 is a major release, what was the minor release number?

@rhpvorderman
Copy link
Contributor Author

rhpvorderman commented Jan 18, 2022

3.11 is a major release, what was the minor release number?

Ah, I see I was using semantic versioning terminology, but obviously that does not apply here.
The bug was introduced in Python 3.11.0 alpha 1.

How shall I describe the news entry? It is a bit silly that the 3.11 final released version will have two entries describing the introduction of a performance improvement and a bugfix in it. So how would I word this?

EDIT. Technically both commits will be in 3.11.0. So is a news entry still necessary?
EDIT2: Ah I see now how the changelog is organized. I will make a news entry.

@rhpvorderman
Copy link
Contributor Author

A news entry was added.

@akulakov
Copy link
Contributor

Thanks for adding it, I think we are almost there.
It's useful to have two news entries for the same reason you would have them if feature was added in 3.10 and bugfix in 3.11 -- users can have a version installed with the feature but before bugfix and we should tell them there's a bug and which exact version to upgrade to if the bug affects them.

@akulakov
Copy link
Contributor

LGTM!

@rhpvorderman
Copy link
Contributor Author

I squashed the commits. 4 commits for 12 lines was a bit too much. The code content is still the same.

@JelleZijlstra
Copy link
Member

#31215 already fixed this, but didn't add tests. I'll merge this PR too to make sure we have tests.

This is now a test-only change, so no need for a NEWS entry.
@rhpvorderman
Copy link
Contributor Author

#31215 already fixed this, [...]

No, this PR already fixed it. Five months ago, four months before that other PR. It has had the bugfix label for 5 months. It is a bit unfortunate that not merging it sooner eventually lead to duplicated effort. But I understand keeping up is hard with the limited amount of people available.

Anyway, I am glad it is in now. I also happened to be the one who introduced the bug (when I made the compress function have quite a lot less overhead). Imagine the shame of having broken something in CPython. Thanks!

@JelleZijlstra
Copy link
Member

#31215 already fixed this, [...]

No, this PR already fixed it. Five months ago, four months before that other PR. It has had the bugfix label for 5 months. It is a bit unfortunate that not merging it sooner eventually lead to duplicated effort. But I understand keeping up is hard with the limited amount of people available.

I'm sorry for the imprecise wording there. There is a lot of open PRs and it's difficult to keep up.

@JelleZijlstra
Copy link
Member

Thanks for your contribution, and sorry again this PR had to wait for so long!

@rhpvorderman
Copy link
Contributor Author

Sorry for my sour initial response. As you can see I had to jump trough quite a lot of hoops to satisfy alukov for this simple one liner. Not that I mind, the PR became much better for it. But for this PR to close after 5 months due to someone else getting a oneliner fix in that did not get the same treatment... Well, it is quite an anti-climax ;-).

Thanks again for merging and I hope you have a nice day!

@rhpvorderman rhpvorderman deleted the bpo46267 branch May 3, 2022 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip news type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants