gh-108494: AC: change parse_file() API

[vstinner]

Revert my change adding 'ns' parameter, add back 'verify' parameter, and add also 'limited_capi' parameter.

• Issue: Add support for the limited C API to Argument Clinic #108494

[serhiy-storchaka]

I would prefer if it specify the version of the limited API, not a simple boolean.

[vstinner]

For now, it only supports Python 3.13. The value can be changed to an int later

[vstinner]

I'm waiting for the review @AlexWaygood, he proposed that change.

[AlexWaygood]

Thanks for fixing this up!

I had a work event all day yesterday and I'm visiting family today; I'd like to take a proper look tomorrow

[AlexWaygood]

If we are using a global DEFAULT_LIMITED_CAPI constant for the default value here, then I think we should also use it for the default value in Clinic.__init__, instead of hardcoding it to False there:

cpython/Tools/clinic/clinic.py

Line 2393 in 0e8b3fc

limited_capi: bool = False,

Though, I'm actually not sure either verify or limited_capi need to have default values. There aren't any places in clinic.py where parse_file is currently called without explicitly passing values to these parameters. And in general, I prefer to avoid having default values unless there's a good reason to have a default value. It forces you to be explicit about the values that are being passed to the function at every callsite, which reduces the potential for accidental bugs to creep in.

[vstinner]

I added a variable for the default to consider changing it tomorrow if AC is shipped as a third party tool.

[AlexWaygood]

I don't think we should be making decisions now on the basis that we might want to ship AC as a third-party tool in the future. Making AC suitable for use in third-party projects would require quite a few changes across Tools/clinic/. It's not currently a goal to make it suitable for use in third-party projects, and I'd like us to have a proper discussion about the pros and cons for doing so before we change that policy. It's pretty clearly documented at the moment that it's only intended as an internal tool: see the note: paragraph at the top of https://docs.python.org/3/howto/clinic.html#argument-clinic-how-to.

But even if we did want to flip the default tomorrow, I don't think the current structure would make it easy to do so. We'd have to change three places in the code:

diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py
index 96ae1e5ad6..5ddaeea855 100755
--- a/Tools/clinic/clinic.py
+++ b/Tools/clinic/clinic.py
@@ -63,7 +63,7 @@

 version = '1'

-DEFAULT_LIMITED_CAPI = False
+DEFAULT_LIMITED_CAPI = True
 NO_VARARG = "PY_SSIZE_T_MAX"
 CLINIC_PREFIX = "__clinic_"
 CLINIC_PREFIXED_ARGS = {
@@ -2390,7 +2390,7 @@ def __init__(
             *,
             filename: str,
             verify: bool = True,
-            limited_capi: bool = False,
+            limited_capi: bool = True,
     ) -> None:
         # maps strings to Parser objects.
         # (instantiated from the "parsers" global.)
@@ -6065,7 +6065,7 @@ def create_cli() -> argparse.ArgumentParser:
     cmdline.add_argument("--exclude", type=str, action="append",
                          help=("a file to exclude in --make mode; "
                                "can be given multiple times"))
-    cmdline.add_argument("--limited", dest="limited_capi", action='store_true',
+    cmdline.add_argument("--no-limited", dest="limited_capi", action='store_false',
                          help="use the Limited C API")
     cmdline.add_argument("filename", metavar="FILE", type=str, nargs="*",
                          help="the list of files to process")

But if we make it so that neither parse_file nor Clinic.__init__ have a default value for the limited_capi parameter, then there will be a "single source of truth" in AC for whether the limited C API is the default or not, and we would only have to make this change when/if we decide to flip the default:

diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py
index 96ae1e5ad6..8866a52f71 100755
--- a/Tools/clinic/clinic.py
+++ b/Tools/clinic/clinic.py
@@ -6065,7 +6065,7 @@ def create_cli() -> argparse.ArgumentParser:
     cmdline.add_argument("--exclude", type=str, action="append",
                          help=("a file to exclude in --make mode; "
                                "can be given multiple times"))
-    cmdline.add_argument("--limited", dest="limited_capi", action='store_true',
+    cmdline.add_argument("--no-limited", dest="limited_capi", action='store_false',
                          help="use the Limited C API")
     cmdline.add_argument("filename", metavar="FILE", type=str, nargs="*",
                          help="the list of files to process")

[vstinner]

If we are using a global DEFAULT_LIMITED_CAPI constant for the default value here, then I think we should also use it for the default value in Clinic.init, instead of hardcoding it to False there: (...)

Done.

If you prefer, I can just remove DEFAULT_LIMITED_CAPI. Changing the default tomorrow is not a big deal.

[vstinner]

@AlexWaygood: Would you mind to review the updated PR? Do you prefer to keep DEFAULT_LIMITED_CAPI, or remove it?

[AlexWaygood]

I still have a bunch of nits: I don't think we need the MockClinic class that was added to the test file in 1dd9510; the types import in the test file is now an unused import; and I still think it would be better to make limited_capi a required argument everywhere. I made #108575 as an alternative PR, which builds on this one :)