Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-95065: Argument Clinic: Add functional tests of deprecated positionals#107768

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commentedAug 8, 2023•
edited by bedevere-bot
Loading

Move the "deprecated positinal" tests from clinic.test.c to
_testclinic.c. Mock PY_VERSION_HEX in order to prevent the generated
compiler warnings/errors to trigger. Put clinic code for deprecated
positionals in Modules/clinic/_testclinic_depr_star.c.h for easy
inspection of the generated code.

…ositionalsMove the "deprecated positinal" tests from clinic.test.c to_testclinic.c. Mock PY_VERSION_HEX in order to prevent the generatedcompiler warnings/errors to trigger. Put clinic code for deprecatedpositionals in Modules/clinic/_testclinic_depr_star.c.h for easyinspection of the generated code.
@erlend-aasland
Copy link
ContributorAuthor

The big diff is caused by moving the generated clinic code.

@erlend-aasland
Copy link
ContributorAuthor

@serhiy-storchaka, IMO this is better than visual inspection of the generated code. We want to prevent regressions because of future refactorings, features and bugfixes.

@AlexWaygood
Copy link
Member

The CI currently isn't running on this PR due to merge conflicts -- I tried building with it locally, but it causes build failures on Windows:

Build FAILED.C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(11,1): error C2133: 'deprecate_positional_pos0_len1__doc__': unknown size [C:\Users\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(98,1): error C2133: 'deprecate_positional_pos0_len2__doc__': unknown size [C:\Users\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(188,1): error C2133: 'deprecate_positional_pos0_len3_with_kwd__doc__': unknown size[C:\Users\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(287,1): error C2133: 'deprecate_positional_pos1_len1_optional__doc__': unknown size[C:\Users\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(383,1): error C2133: 'deprecate_positional_pos1_len1__doc__': unknown size [C:\Users\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(473,1): error C2133: 'deprecate_positional_pos1_len2_with_kwd__doc__': unknown size[C:\Users\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(572,1): error C2133: 'deprecate_positional_pos2_len1__doc__': unknown size [C:\Users\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(664,1): error C2133: 'deprecate_positional_pos2_len2__doc__': unknown size [C:\Users\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]C:\Users\alexw\coding\cpython\Modules\clinic\_testclinic_depr_star.c.h(758,1): error C2133: 'deprecate_positional_pos2_len3_with_kwd__doc__': unknown size[C:\Users\alexw\coding\cpython\PCbuild\_testclinic.vcxproj]    0 Warning(s)    9 Error(s)

@erlend-aasland
Copy link
ContributorAuthor

The CI currently isn't running on this PR due to merge conflicts -- I tried building with it locally, but it causes build failures on Windows:

Does it still happen?

output push
destination deprstar new file '{dirname}/clinic/_testclinic_depr_star.c.h'
output everything deprstar
#output methoddef_ifndef buffer 1
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

The output directive has a bug that prevents us from specifying the stack index. We should fix that in a separate PR.

@AlexWaygood
Copy link
Member

The CI currently isn't running on this PR due to merge conflicts -- I tried building with it locally, but it causes build failures on Windows:

Does it still happen?

Nope! Builds fine locally now :)

erlend-aasland reacted with hooray emoji

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Would not be better to use version like 100.200 to avoid rewriting tests every year?

@erlend-aasland
Copy link
ContributorAuthor

Would not be better to use version like 100.200 to avoid rewriting tests every year?

No need, we mock the version.

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Do the test fail if change the stacklevel argument inPyErr_WarnEx() from 1 to 2?

@erlend-aasland
Copy link
ContributorAuthor

Do the test fail if change the stacklevel argument inPyErr_WarnEx() from 1 to 2?

They do not. The same filename is produced for both stack levels.

@serhiy-storchaka
Copy link
Member

And wrong stacklevel is a common error. Even your original code did have it wrong initially. Therefore, I think it is important to check it using one of the methods I suggested above.

erlend-aasland and AlexWaygood reacted with thumbs up emoji

@erlend-aasland
Copy link
ContributorAuthor

Note: we will get merge conflicts from#107808.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Member

@AlexWaygoodAlexWaygood left a comment

Choose a reason for hiding this comment

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

I agree with Serhiy that explicitly testing the stacklevel would be good. Other than that, this is looking much better now, thanks! The readabiliy is much improved :)

erlend-aasland reacted with thumbs up emoji
Copy link
Member

@AlexWaygoodAlexWaygood 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 happy if Serhiy's happy!

erlend-aasland reacted with hooray emoji
Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Few nitpicks and LGTM.

erlend-aasland reacted with hooray emoji
@erlend-aaslanderlend-aaslandenabled auto-merge (squash)August 10, 2023 06:54
@erlend-aasland
Copy link
ContributorAuthor

Thank you Serhiy and Alex; this PR is in much better shape now! With this, we've got a nice framework for similar tests in place. I'll start working on what's left in clinic.test.c.

@erlend-aaslanderlend-aasland merged commit39ef93e intopython:mainAug 10, 2023
@erlend-aaslanderlend-aasland deleted the clinic/better-depr-tests branchAugust 10, 2023 07:35
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@AlexWaygoodAlexWaygoodAlexWaygood approved these changes

@sobolevnsobolevnAwaiting requested review from sobolevn

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@erlend-aasland@AlexWaygood@serhiy-storchaka@bedevere-bot
[8]ページ先頭
©2009-2025 Movatter.jp