Skip to content

Conversation

fatelei
Copy link

@fatelei fatelei commented Oct 14, 2025

gh-140025 queue.SimpleQueue.sizeof() ignores the underlying data structure

@bedevere-app
Copy link

bedevere-app bot commented Oct 14, 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

This comment was marked as duplicate.

@bedevere-app

This comment was marked as duplicate.

@picnixz picnixz changed the title fix: gh-140025 queue.SimpleQueue.__sizeof__() ignores the underlying … gh-140025: fix queue.SimpleQueue.__sizeof__() computation Oct 14, 2025
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Per the bot, please add a news entry. Please also avoid force-pushing; it makes things harder to review and we squash at the end anyway.

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Overall feedback:

  • Implement the tests in PySimpleQueueTest and CSimpleQueueTest. They will be different becauss the impmementation is different.
  • Remove messages when assertions fail and when the condition being tested is self-explanatory.
  • Remove commenta for self-explanatory code.

@bedevere-app
Copy link

bedevere-app bot commented Oct 15, 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 Oct 16, 2025

Thanks for making the requested changes!

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

@bedevere-app bedevere-app bot requested a review from picnixz October 16, 2025 04:00
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

The changes were not those that I wanted. I wanted you to test sizeof() for the pure python and C implementations separately.

@bedevere-app
Copy link

bedevere-app bot commented Oct 16, 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.

@fatelei
Copy link
Author

fatelei commented Oct 16, 2025

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Oct 16, 2025

Thanks for making the requested changes!

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

@bedevere-app bedevere-app bot requested a review from picnixz October 16, 2025 05:20
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Please be more careful with review feedback. I also feel that this PR was AI generated because the previous version tested equality which was not asked although one of my comment mentioned it. I am at the airport and will board in a few minutes so I will be less available for the next few days. If another core dev considers the PR to be in a mergeable state, please proceed even if I didn't leave another review.

self.type2test = self.queue.SimpleQueue
super().setUp()

def test_is_default(self):
Copy link
Member

Choose a reason for hiding this comment

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

Why were those tests removed????

Copy link
Author

Choose a reason for hiding this comment

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

no need

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean, no need? why touch other tests that are not part of what the PR should address? please only make relevant changes and do not touch unrelated code in the same PR.

@bedevere-app
Copy link

bedevere-app bot commented Oct 16, 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.

@fatelei
Copy link
Author

fatelei commented Oct 16, 2025

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Oct 16, 2025

Thanks for making the requested changes!

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

@picnixz
Copy link
Member

picnixz commented Oct 16, 2025

The changes made are more and more unrelated and I do not understand why. As I am now going offline I will ask another core dev to review this thoroughly and check if my comments are addressed in the final version of that PR. I leave them the responsibility of removing the label as well

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.

7 participants