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.

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

gh-108494: AC: change parse_file() API #108504

Closed
wants to merge 2 commits into from
Closed

gh-108494: AC: change parse_file() API #108504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b1ae97
gh-108494: AC: change parse_file() API
vstinner Aug 26, 2023
e9f70f2
Clinic constructor uses DEFAULT_LIMITED_CAPI
vstinner Aug 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions Lib/test/test_clinic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,6 @@
from clinic import DSLParser


def default_namespace():
ns = types.SimpleNamespace()
ns.force = False
ns.limited_capi = clinic.DEFAULT_LIMITED_CAPI
return ns


def _make_clinic(*, filename='clinic_tests'):
clang = clinic.CLanguage(None)
c = clinic.Clinic(clang, filename=filename)
Expand Down Expand Up @@ -704,9 +697,8 @@ def expect_parsing_failure(
self, *, filename, expected_error, verify=True, output=None
):
errmsg = re.escape(dedent(expected_error).strip())
ns = default_namespace()
with self.assertRaisesRegex(clinic.ClinicError, errmsg):
clinic.parse_file(filename, ns=ns)
clinic.parse_file(filename)

def test_parse_file_no_extension(self) -> None:
self.expect_parsing_failure(
Expand Down
13 changes: 7 additions & 6 deletions Tools/clinic/clinic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2415,7 +2415,7 @@ def __init__(
*,
filename: str,
verify: bool = True,
limited_capi: bool = False,
limited_capi: bool = DEFAULT_LIMITED_CAPI,
) -> None:
# maps strings to Parser objects.
# (instantiated from the "parsers" global.)
Expand Down Expand Up @@ -2612,11 +2612,10 @@ def __repr__(self) -> str:
def parse_file(
filename: str,
*,
ns: argparse.Namespace,
output: str | None = None,
verify: bool = True,
limited_capi: bool = DEFAULT_LIMITED_CAPI,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Comment on lines +2616 to +2617
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

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 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")

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

) -> None:
verify = not ns.force
limited_capi = ns.limited_capi
if not output:
output = filename

Expand Down Expand Up @@ -6190,7 +6189,8 @@ def run_clinic(parser: argparse.ArgumentParser, ns: argparse.Namespace) -> None:
continue
if ns.verbose:
print(path)
parse_file(path, ns=ns)
parse_file(path,
verify=not ns.force, limited_capi=ns.limited_capi)
return

if not ns.filename:
Expand All @@ -6202,7 +6202,8 @@ def run_clinic(parser: argparse.ArgumentParser, ns: argparse.Namespace) -> None:
for filename in ns.filename:
if ns.verbose:
print(filename)
parse_file(filename, output=ns.output, ns=ns)
parse_file(filename, output=ns.output,
verify=not ns.force, limited_capi=ns.limited_capi)


def main(argv: list[str] | None = None) -> NoReturn:
Expand Down