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

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

Closed
corona10 opened this issue May 14, 2023 · 13 comments
Closed

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

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

Comments

@corona10
Copy link
Member

corona10 commented May 14, 2023

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 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
@sunmy2019
Copy link
Member

LGTM

@corona10
Copy link
Member Author

I am working on test_exceptions.

@sobolevn
Copy link
Member

I am working on _testcapi/watchers.c 👍

@erlend-aasland
Copy link
Contributor

erlend-aasland commented May 15, 2023

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
* 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
Member

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
* 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
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
carljm added a commit to carljm/cpython that referenced this issue May 16, 2023
* 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
* gh-104469: Update README.txt for _testcapi

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Kirill Podoprigora <[email protected]>
carljm added a commit to carljm/cpython that referenced this issue May 17, 2023
* main: (26 commits)
  pythonGH-101520: Move tracemalloc functionality into core, leaving interface in Modules. (python#104508)
  typing: Add more tests for TypeVar (python#104571)
  pythongh-104572: Improve error messages for invalid constructs in PEP 695 contexts (python#104573)
  typing: Use PEP 695 syntax in typing.py (python#104553)
  pythongh-102153: Start stripping C0 control and space chars in `urlsplit` (python#102508)
  pythongh-104469: Update README.txt for _testcapi (pythongh-104529)
  pythonGH-103092: isolate `_elementtree` (python#104561)
  pythongh-104050: Add typing to Argument Clinic converters (python#104547)
  pythonGH-103906: Remove immortal refcounting in the interpreter (pythonGH-103909)
  pythongh-87474: Fix file descriptor leaks in subprocess.Popen (python#96351)
  pythonGH-103092: isolate `pyexpat`  (python#104506)
  pythongh-75367: Fix data descriptor detection in inspect.getattr_static (python#104517)
  pythongh-104050: Add more annotations to `Tools/clinic.py` (python#104544)
  pythongh-104555: Fix isinstance() and issubclass() for runtime-checkable protocols that use PEP 695 (python#104556)
  pythongh-103865: add monitoring support to LOAD_SUPER_ATTR (python#103866)
  CODEOWNERS: Assign new PEP 695 files to myself (python#104551)
  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)
  ...
JelleZijlstra pushed a commit to JelleZijlstra/cpython that referenced this issue May 18, 2023
* pythongh-104469: Update README.txt for _testcapi

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Kirill Podoprigora <[email protected]>
@littlebutt
Copy link
Contributor

Hi @corona10 , I worked on worked on vectorcall.c and PRed just now. But some test cases failed because of missing arguments. I'm not sure if AC introduced the problem. Thanks.

@serhiy-storchaka
Copy link
Member

Could you please stop this?

It significantly reduces readability to me.

It significantly increases the number of lines in Modules/_testcapi/dict.c:

 Modules/_testcapi/dict.c | 412 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------
 1 file changed, 291 insertions(+), 121 deletions(-)

And this is not counting the large amount of code added to another file.

I have a limited field of vision and can't see anything beyond a limited number of lines of code. Before that, test functions were compact. After these changes, they will exceed my limit. And if I need to understand what exactly the function does, I have to look for the corresponding code in the generated file and then go back to the previous file, but when I look from the file I lose the context and I have to reread each line on the screen again to find the right place.

These changes are not friendly to the visually impaired.

@corona10
Copy link
Member Author

corona10 commented Aug 11, 2023

@serhiy-storchaka cc @erlend-aasland

These changes are not friendly to the visually impaired.

Okay, I should respect your physical situation.
Today was PyCon KR sprint day as I shared, and I tried to provide kinds of easy issues who want to write actual code works.

As you requested, I will close this issue and not progress this issue anymore.
I will drop the following PRs except

The following one will be merged #107857 this is not many changes.
I would like to request you to understand the special situation of today,

@serhiy-storchaka
Copy link
Member

Thank you for respecting my physical situation @corona10.

These testing files are different from "normal" code using Argument Clinic, because they contain a large number of very short (in comparison to clinic decoration) and very similar functions. "Normal" code usually have larger functions, and the code is visually different. So I have less problems with other files.

corona10 added a commit to corona10/cpython that referenced this issue Aug 14, 2023
corona10 added a commit to corona10/cpython that referenced this issue Aug 14, 2023
corona10 added a commit to corona10/cpython that referenced this issue Aug 14, 2023
corona10 added a commit to corona10/cpython that referenced this issue Aug 14, 2023
corona10 added a commit that referenced this issue Aug 14, 2023
gh-107951)

Revert "gh-104469 : Convert _testcapi/vectorcall_limited.c to use AC (gh-107857)"

This reverts commit 2e27da1.
iritkatriel pushed a commit to iritkatriel/cpython that referenced this issue Aug 16, 2023
…se AC … (pythongh-107951)

Revert "pythongh-104469 : Convert _testcapi/vectorcall_limited.c to use AC (pythongh-107857)"

This reverts commit 2e27da1.
vstinner added a commit to vstinner/cpython that referenced this issue Sep 21, 2023
vstinner added a commit to vstinner/cpython that referenced this issue Sep 21, 2023
vstinner added a commit to vstinner/cpython that referenced this issue Sep 21, 2023
vstinner added a commit to vstinner/cpython that referenced this issue Sep 21, 2023
Fix make check-c-globals: complete USE_LIMITED_C_API list of the
c-analyzer.
vstinner added a commit that referenced this issue Sep 21, 2023
Fix make check-c-globals: complete USE_LIMITED_C_API list of the
c-analyzer.
csm10495 pushed a commit to csm10495/cpython that referenced this issue Sep 28, 2023
…thon#109690)

Fix make check-c-globals: complete USE_LIMITED_C_API list of the
c-analyzer.
csm10495 pushed a commit to csm10495/cpython that referenced this issue Sep 28, 2023
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
…thon#109690)

Fix make check-c-globals: complete USE_LIMITED_C_API list of the
c-analyzer.
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir topic-C-API
Projects
None yet
Development

No branches or pull requests

6 participants