Skip to content

gh-68552: fix defects policy #138579

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.

Sign up for GitHub

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

Jump to bottom
Merged
bitdancer merged 4 commits into from
Dec 6, 2025
Merged

gh-68552: fix defects policy #138579

bitdancer merged 4 commits into from
Dec 6, 2025

Conversation

@nilleb
Copy link
Contributor

@nilleb nilleb commented Sep 6, 2025 •
edited by bedevere-app bot
Loading

I am reporting in this pull request the fix made by @vadmium a long while ago.
(It wasn't clear which was the issue author expectation (@bitdancer) but I hope this will help taking a decision about the issue and eventually clean up the backlog.)

Thank you!

@nilleb nilleb requested a review from a team as a code owner September 6, 2025 12:27
@bedevere-app
Copy link

bedevere-app bot commented Sep 6, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app bedevere-app bot mentioned this pull request Sep 6, 2025
Closed
bitdancer
bitdancer requested changes Nov 6, 2025
View reviewed changes
Copy link
Member

@bitdancer bitdancer 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 working on this.

Lib/test/test_email/test_defect_handling.py Outdated
yield

def get_defects(self, obj):
return obj.defects
Copy link
Member

@bitdancer bitdancer Nov 6, 2025

Choose a reason for hiding this comment

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

It would be better to move this method to the TestCompat32 class.

Copy link
Contributor Author

@nilleb nilleb Nov 30, 2025

Choose a reason for hiding this comment

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

I have moved this to both TestCompat32 and TestDefectRaising - it was needed there as well.

Lib/test/test_email/test_defect_handling.py Outdated
msg = self._str_msg(string)
self.assertEqual(len(self.get_defects(msg)), 1)
self.assertDefectsEqual(self.get_defects(msg), [defect])
return msg
Copy link
Member

@bitdancer bitdancer Nov 6, 2025

Choose a reason for hiding this comment

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

While this method does point toward a nice cleanup of the existing tests, I would do that cleanup in a different way. For this PR I would prefer to follow the existing pattern. Please remove this method and in-line the call to _raise_point following the pattern in the existing test methods.

Lib/test/test_email/test_defect_handling.py Outdated
if msg:
headers = [('Subject', 'Dummy subject'), ('To', 'abc')]
self.assertEqual(msg.items(), headers)
self.assertEqual(msg.get_payload(), 'body\r\n')
Copy link
Member

@bitdancer bitdancer Nov 6, 2025

Choose a reason for hiding this comment

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

As noted above, let's rewrite these to mirror the existing test pattern.

Lib/test/test_email/test_email.py Outdated
errors.NoBoundaryInMultipartDefect)
self.assertIsInstance(msg.defects[1],
errors.MultipartInvariantViolationDefect)

Copy link
Member

@bitdancer bitdancer Nov 6, 2025

Choose a reason for hiding this comment

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

The test methods test_same_boundary_inner_outer and test_mulltipart_no_boundary are now also redundant and can be deleted.

@bedevere-app
Copy link

bedevere-app bot commented Nov 6, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@bedevere-app
Copy link

bedevere-app bot commented Nov 30, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@nilleb
Copy link
Contributor Author

nilleb commented Nov 30, 2025

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Nov 30, 2025

Thanks for making the requested changes!

@bitdancer: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from bitdancer November 30, 2025 08:06
@bitdancer
Copy link
Member

bitdancer commented Dec 4, 2025

Looks good, but it needs a news entry. Something like "MisplacedEnvelopeHeaderDefect and 'Missing header name' defects are now correctly passed to the handle_defect method of policy". With appropriate formatting, of course.

@nilleb
Copy link
Contributor Author

nilleb commented Dec 4, 2025

hi @bitdancer !
thanks a lot for your review - I think I did it - This was a premiere using RST, so my apologies if I got something wrong. (I noticed that the references to FeedParser have evolved over time, I think I got the right one in my freshest commit).

bitdancer
bitdancer approved these changes Dec 5, 2025
View reviewed changes
Copy link
Member

@bitdancer bitdancer left a comment

Choose a reason for hiding this comment

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

This looks fine. Technically you could link the defect and the method to the docs, but I don't think we need to go that far.

@bitdancer bitdancer merged commit 9d707d8 into python:main Dec 6, 2025
49 of 50 checks passed
@bitdancer bitdancer added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Dec 6, 2025
@miss-islington-app
Copy link

miss-islington-app bot commented Dec 6, 2025

Thanks @nilleb for the PR, and @bitdancer for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

@miss-islington-app
Copy link

miss-islington-app bot commented Dec 6, 2025

Thanks @nilleb for the PR, and @bitdancer for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 6, 2025
@nilleb @vadmium @miss-islington
pythongh-68552: fix defects policy (pythonGH-138579)
894daaa 
Extend defect handling via policy to a couple of missed defects.

---------
(cherry picked from commit 9d707d8)

Co-authored-by: Ivo Bellin Salarin <nilleb@users.noreply.github.com>
Co-authored-by: Martin Panter <vadmium@users.noreply.github.com>
Co-authored-by: Ivo Bellin Salarin <ivo@nilleb.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 6, 2025
@nilleb @vadmium @miss-islington
pythongh-68552: fix defects policy (pythonGH-138579)
976841c 
Extend defect handling via policy to a couple of missed defects.

---------
(cherry picked from commit 9d707d8)

Co-authored-by: Ivo Bellin Salarin <nilleb@users.noreply.github.com>
Co-authored-by: Martin Panter <vadmium@users.noreply.github.com>
Co-authored-by: Ivo Bellin Salarin <ivo@nilleb.com>
@bedevere-app
Copy link

bedevere-app bot commented Dec 6, 2025

GH-142366 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Dec 6, 2025
@bedevere-app
Copy link

bedevere-app bot commented Dec 6, 2025

GH-142367 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Dec 6, 2025
@nilleb nilleb deleted the fix/gh-68552 branch December 8, 2025 09:42
hugovk pushed a commit that referenced this pull request Dec 9, 2025
@miss-islington @nilleb @vadmium
[3.14] gh-68552: fix defects policy (GH-138579) (#142366)
f200776 
Co-authored-by: Ivo Bellin Salarin <nilleb@users.noreply.github.com>
Co-authored-by: Martin Panter <vadmium@users.noreply.github.com>
Co-authored-by: Ivo Bellin Salarin <ivo@nilleb.com>
hugovk pushed a commit that referenced this pull request Dec 9, 2025
@miss-islington @nilleb @vadmium
[3.13] gh-68552: fix defects policy (GH-138579) (#142367)
b57d695 
Co-authored-by: Ivo Bellin Salarin <nilleb@users.noreply.github.com>
Co-authored-by: Martin Panter <vadmium@users.noreply.github.com>
Co-authored-by: Ivo Bellin Salarin <ivo@nilleb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bitdancer bitdancer bitdancer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nilleb @bitdancer