Skip to content
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 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

Improve the readability and maintainability of test_capi using the AC #104469

Open
corona10 opened this issue May 14, 2023 · 9 comments
Open

Improve the readability and maintainability of test_capi using the AC #104469

corona10 opened this issue May 14, 2023 · 9 comments
Labels
tests Tests in the Lib/test dir topic-C-API

Comments

@corona10
Copy link
Member

corona10 commented May 14, 2023 •
edited by bedevere-bot

Currently, most of test_capi modules do not use the Argument Clinic tool.
As a result, we manually implement docstrings containing explanations for the test codes and handle parameter parsing manually: PyArg_ParseTuple .
To maintain code consistency in test_capi, I suggest using the Argument Clinic tool.

While some might criticize this as code churn, I believe it is necessary for maintaining consistent test code writing practices. I will attach a sample PR to illustrate how it can help improve the understanding of the test code. I hope this will help clarify the rationale behind it.

cc @erlend-aasland @sobolevn

Linked PRs
@corona10 corona10 added tests Tests in the Lib/test dir topic-C-API labels May 14, 2023
corona10 added a commit to corona10/cpython that referenced this issue May 14, 2023
@corona10
pythongh-104469 Convert _testcapi/flaot.c to use AC
f0ce379
@bedevere-bot bedevere-bot mentioned this issue May 14, 2023
Merged
@corona10 corona10 changed the title Improve the readability and maintainability of test_capi using the Argument Clinic tool Improve the readability and maintainability of test_capi using the AC May 14, 2023
corona10 added a commit to corona10/cpython that referenced this issue May 14, 2023
@corona10
pythongh-104469 Convert _testcapi/flaot.c to use AC
f7f249c
@sunmy2019
Copy link
Contributor

LGTM

corona10 added a commit that referenced this issue May 15, 2023
@corona10
gh-104469 Convert _testcapi/float.c to use AC (gh-104470)
48b3617
@corona10
Copy link
Member Author

I am working on test_exceptions.

corona10 added a commit to corona10/cpython that referenced this issue May 15, 2023
@corona10
pythongh-104469 Convert _testcapi/exceptions to use AC
4912e14
@bedevere-bot bedevere-bot mentioned this issue May 15, 2023
Open
@sobolevn
Copy link
Member

I am working on _testcapi/watchers.c 👍

sobolevn added a commit to sobolevn/cpython that referenced this issue May 15, 2023
@sobolevn
pythongh-104469: Convert _testcapi/watchers.c to use AC
559d74f
@bedevere-bot bedevere-bot mentioned this issue May 15, 2023
Merged
@erlend-aasland
Copy link
Contributor

erlend-aasland commented May 15, 2023 •
edited

Let's establish some guidelines:

  • For the sake of readability (which is one of the goals here), please add a newline between the argument spec and the docstring
  • If purpose is obvious given a function name, omit the docstring
  • If a description is needed, make sure the added docstring clearly and succinctly describes purpose of the function
  • DRY, use the clone feature of Argument Clinic
  • Try to avoid adding new interned strings; reuse existing parameter names if possible and use the as feature to override the C name

cc. @sobolevn @corona10

@sobolevn
Copy link
Member

Agreed, updated my PR.

@corona10
Copy link
Member Author

Let's establish some guidelines:

+1, Super great
Update README.txt? (I prefer to update it to README.md either)
or publish it to https://devguide.python.org/developer-workflow/c-api/#c-api here?

carljm added a commit to carljm/cpython that referenced this issue May 15, 2023
@carljm
Merge branch 'main' into supermon
343867a 
* main: (29 commits)
  pythongh-101819: Fix _io clinic input for unused base class method stubs (python#104418)
  pythongh-101819: Isolate `_io` (python#101948)
  Bump mypy from 1.2.0 to 1.3.0 in /Tools/clinic (python#104501)
  pythongh-104494: Update certain Tkinter pack/place tests for Tk 8.7 errors (python#104495)
  pythongh-104050: Run mypy on `clinic.py` in CI (python#104421)
  pythongh-104490: Consistently define phony make targets (python#104491)
  pythongh-67056: document that registering/unregistering an atexit func from within an atexit func is undefined (python#104473)
  pythongh-104487: PYTHON_FOR_REGEN must be minimum Python 3.10 (python#104488)
  pythongh-101282: move BOLT config after PGO (pythongh-104493)
  pythongh-104469 Convert _testcapi/float.c to use AC (pythongh-104470)
  pythongh-104456: Fix ref leak in _ctypes.COMError (python#104457)
  pythongh-98539: Make _SSLTransportProtocol.abort() safe to call when closed (python#104474)
  pythongh-104337: Clarify random.gammavariate doc entry  (python#104410)
  Minor improvements to typing docs (python#104465)
  pythongh-87092: avoid gcc warning on uninitialized struct field in assemble.c (python#104460)
  pythonGH-71383: IDLE - Document testing subsets of modules (python#104463)
  pythongh-104454: Fix refleak in AttributeError_reduce (python#104455)
  pythongh-75710: IDLE - add docstrings and comments to editor module (python#104446)
  pythongh-91896: Revert some very noisy DeprecationWarnings for `ByteString` (python#104424)
  Add a mention of PYTHONBREAKPOINT to breakpoint() docs (python#104430)
  ...
@erlend-aasland
Copy link
Contributor

Update README.txt?

Definitely! 🙂

(I prefer to update it to README.md either)

I'd just leave it as .txt unless there's a specific reason to reformat it.

or publish it to https://devguide.python.org/developer-workflow/c-api/#c-api here?

Yes, perhaps we should add some general AC advice to the devguide. Perhaps the first step is to link from the devguide to the AC How-to. We should also overhaul the latter considerably.

@sunmy2019
Copy link
Contributor

there's a specific reason

README.md can be loaded by GitHub 🤣

carljm added a commit to carljm/cpython that referenced this issue May 15, 2023
@carljm
Merge branch 'main' into cached_property_docs
452d9f5 
* main: (204 commits)
  pythongh-101819: Fix _io clinic input for unused base class method stubs (python#104418)
  pythongh-101819: Isolate `_io` (python#101948)
  Bump mypy from 1.2.0 to 1.3.0 in /Tools/clinic (python#104501)
  pythongh-104494: Update certain Tkinter pack/place tests for Tk 8.7 errors (python#104495)
  pythongh-104050: Run mypy on `clinic.py` in CI (python#104421)
  pythongh-104490: Consistently define phony make targets (python#104491)
  pythongh-67056: document that registering/unregistering an atexit func from within an atexit func is undefined (python#104473)
  pythongh-104487: PYTHON_FOR_REGEN must be minimum Python 3.10 (python#104488)
  pythongh-101282: move BOLT config after PGO (pythongh-104493)
  pythongh-104469 Convert _testcapi/float.c to use AC (pythongh-104470)
  pythongh-104456: Fix ref leak in _ctypes.COMError (python#104457)
  pythongh-98539: Make _SSLTransportProtocol.abort() safe to call when closed (python#104474)
  pythongh-104337: Clarify random.gammavariate doc entry  (python#104410)
  Minor improvements to typing docs (python#104465)
  pythongh-87092: avoid gcc warning on uninitialized struct field in assemble.c (python#104460)
  pythonGH-71383: IDLE - Document testing subsets of modules (python#104463)
  pythongh-104454: Fix refleak in AttributeError_reduce (python#104455)
  pythongh-75710: IDLE - add docstrings and comments to editor module (python#104446)
  pythongh-91896: Revert some very noisy DeprecationWarnings for `ByteString` (python#104424)
  Add a mention of PYTHONBREAKPOINT to breakpoint() docs (python#104430)
  ...
@erlend-aasland
Copy link
Contributor

there's a specific reason

README.md can be loaded by GitHub 🤣

So can .txt and .rst

erlend-aasland pushed a commit that referenced this issue May 15, 2023
@sobolevn
gh-104469: Convert _testcapi/watchers.c to use Argument Clinic (#104503)
456d566 
Remove boilerplate code by converting the following functions:

- _testcapi.watch_dict
- _testcapi.unwatch_dict
- _testcapi.watch_type
- _testcapi.unwatch_type
- _testcapi.set_func_defaults_via_capi
- _testcapi.set_func_kwdefaults_via_capi
corona10 added a commit to corona10/cpython that referenced this issue May 16, 2023
@corona10 @erlend-aasland
pythongh-104469: Update README.txt for _testcapi
f62fb02 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@bedevere-bot bedevere-bot mentioned this issue May 16, 2023
Merged
carljm added a commit to carljm/cpython that referenced this issue May 16, 2023
@carljm
Merge branch 'main' into supermon
a6dd626 
* main:
  pythonGH-104510: Fix refleaks in `_io` base types (python#104516)
  pythongh-104539: Fix indentation error in logging.config.rst (python#104545)
  pythongh-104050: Don't star-import 'types' in Argument Clinic (python#104543)
  pythongh-104050: Add basic typing to CConverter in clinic.py (python#104538)
  pythongh-64595: Fix write file logic in Argument Clinic (python#104507)
  pythongh-104523: Inline minimal PGO rules (python#104524)
  pythongh-103861: Fix Zip64 extensions not being properly applied in some cases (python#103863)
  pythongh-69152: add method get_proxy_response_headers to HTTPConnection class (python#104248)
  pythongh-103763: Implement PEP 695 (python#103764)
  pythongh-104461: Run tkinter test_configure_screen on X11 only (pythonGH-104462)
  pythongh-104469: Convert _testcapi/watchers.c to use Argument Clinic (python#104503)
  pythongh-104482: Fix error handling bugs in ast.c (python#104483)
  pythongh-104341: Adjust tstate_must_exit() to Respect Interpreter Finalization (pythongh-104437)
  pythonGH-102613: Fix recursion error from `pathlib.Path.glob()` (pythonGH-104373)
corona10 added a commit that referenced this issue May 17, 2023
@corona10 @erlend-aasland @Eclips4
gh-104469: Update README.txt for _testcapi (gh-104529)
b58bc8c 
* gh-104469: Update README.txt for _testcapi

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
tests Tests in the Lib/test dir topic-C-API
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@sobolevn @corona10 @erlend-aasland @sunmy2019