Skip to content

gh-130197: Remove obsolete pygettext tests #130198

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
Closed
wants to merge 5 commits into from
Closed

gh-130197: Remove obsolete pygettext tests #130198

wants to merge 5 commits into from

Conversation

StanFromIreland
Copy link
Contributor

@StanFromIreland StanFromIreland commented Feb 16, 2025 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot added tests Tests in the Lib/test dir awaiting review labels Feb 16, 2025
@bedevere-app bedevere-app bot mentioned this pull request Feb 16, 2025
Open
18 tasks
@StanFromIreland
Copy link
Contributor Author

Requesting @tomasr8

picnixz
picnixz previously requested changes Feb 16, 2025
View reviewed changes
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.

I don't think we need to remove those tests. They are testing that --docstring works from end-to-end. So I think we can keep them.

@bedevere-app

This comment was marked as resolved.

Sign in to view
@StanFromIreland
Copy link
Contributor Author

@tomasr8 what was the plan for the future of it? Deprecate the option?

@picnixz picnixz dismissed their stale review February 16, 2025 21:07

Considering the issue discussion, let's remove the tests while trying to have an E2E test for --docstring

tomasr8
tomasr8 reviewed Feb 17, 2025
View reviewed changes
picnixz
picnixz reviewed Feb 24, 2025
View reviewed changes
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.

I'm no expert in pygettext, so I'll let @tomasr8 and @serhiy-storchaka decide. However, Serhiy wanted to keep all tests I think (see #130197 (comment)) so I think we can also close this PR but keep the issue to improve the test coverage instead (unless there's another issue for that)

@picnixz picnixz added the pending The issue will be closed if no feedback is provided label Feb 24, 2025
@tomasr8
Copy link
Member

tomasr8 commented Feb 24, 2025

Yes let's close this. I think Serhiy is right in the sense that even though the tests are currently not doing anything, if we ever change the implementation (though unlikely), they will be useful again to prevent potential regressions. Sorry @StanFromIreland for the extra work :/

I'll update the issue so we can reuse it for improving the test coverage, I think it's better than opening a new one.

@tomasr8 tomasr8 closed this Feb 24, 2025
@AA-Turner AA-Turner removed the pending The issue will be closed if no feedback is provided label Apr 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@picnixz picnixz picnixz left review comments

@tomasr8 tomasr8 Awaiting requested review from tomasr8

Assignees
No one assigned
Labels
awaiting review skip news tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@StanFromIreland @tomasr8 @picnixz @AA-Turner