Skip to content

gh-104683: Argument clinic: cleanup DLSParser state_foo methods #107543

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

Merged
merged 1 commit into from
Aug 1, 2023
Merged
Changes from all commits
Commits
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
33 changes: 11 additions & 22 deletions Tools/clinic/clinic.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
NamedTuple,
NoReturn,
Protocol,
TypeGuard,
TypeVar,
cast,
overload,
Expand Down Expand Up @@ -4389,7 +4388,7 @@ def dedent(self, line: str) -> str:
return line[indent:]


StateKeeper = Callable[[str | None], None]
StateKeeper = Callable[[str], None]
ConverterArgs = dict[str, Any]

class ParamState(enum.IntEnum):
Expand Down Expand Up @@ -4611,9 +4610,7 @@ def parse(self, block: Block) -> None:
fail('Tab characters are illegal in the Clinic DSL.\n\t' + repr(line), line_number=block_start)
self.state(line)

self.next(self.state_terminal)
self.state(None)

self.do_post_block_processing_cleanup()
block.output.extend(self.clinic.language.render(self.clinic, block.signatures))

if self.preserve_output:
Expand All @@ -4622,10 +4619,7 @@ def parse(self, block: Block) -> None:
block.output = self.saved_output

@staticmethod
def valid_line(line: str | None) -> TypeGuard[str]:
if line is None:
return False

def valid_line(line: str) -> bool:
# ignore comment-only lines
if line.lstrip().startswith('#'):
return False
Expand All @@ -4651,7 +4645,7 @@ def next(
if line is not None:
self.state(line)

def state_dsl_start(self, line: str | None) -> None:
def state_dsl_start(self, line: str) -> None:
# self.block = self.ClinicOutputBlock(self)
if not self.valid_line(line):
return
Expand All @@ -4669,7 +4663,7 @@ def state_dsl_start(self, line: str | None) -> None:

self.next(self.state_modulename_name, line)

def state_modulename_name(self, line: str | None) -> None:
def state_modulename_name(self, line: str) -> None:
# looking for declaration, which establishes the leftmost column
# line should be
# modulename.fnname [as c_basename] [-> return annotation]
Expand Down Expand Up @@ -4858,7 +4852,7 @@ def state_modulename_name(self, line: str | None) -> None:
# separate boolean state variables.) The states are defined in the
# ParamState class.

def state_parameters_start(self, line: str | None) -> None:
def state_parameters_start(self, line: str) -> None:
if not self.valid_line(line):
return

Expand All @@ -4880,7 +4874,7 @@ def to_required(self) -> None:
for p in self.function.parameters.values():
p.group = -p.group

def state_parameter(self, line: str | None) -> None:
def state_parameter(self, line: str) -> None:
assert isinstance(self.function, Function)

if not self.valid_line(line):
Expand Down Expand Up @@ -5263,7 +5257,7 @@ def parse_slash(self, function: Function) -> None:
"positional-only parameters, which is unsupported.")
p.kind = inspect.Parameter.POSITIONAL_ONLY

def state_parameter_docstring_start(self, line: str | None) -> None:
def state_parameter_docstring_start(self, line: str) -> None:
assert self.indent.margin is not None, "self.margin.infer() has not yet been called to set the margin"
self.parameter_docstring_indent = len(self.indent.margin)
assert self.indent.depth == 3
Expand All @@ -5272,9 +5266,7 @@ def state_parameter_docstring_start(self, line: str | None) -> None:
# every line of the docstring must start with at least F spaces,
# where F > P.
# these F spaces will be stripped.
def state_parameter_docstring(self, line: str | None) -> None:
assert line is not None

def state_parameter_docstring(self, line: str) -> None:
stripped = line.strip()
if stripped.startswith('#'):
return
Expand Down Expand Up @@ -5302,9 +5294,8 @@ def state_parameter_docstring(self, line: str | None) -> None:
last_parameter.docstring = new_docstring

# the final stanza of the DSL is the docstring.
def state_function_docstring(self, line: str | None) -> None:
def state_function_docstring(self, line: str) -> None:
assert self.function is not None
assert line is not None

if self.group:
fail("Function " + self.function.name + " has a ] without a matching [.")
Expand Down Expand Up @@ -5573,12 +5564,10 @@ def add_parameter(text: str) -> None:

return docstring

def state_terminal(self, line: str | None) -> None:
def do_post_block_processing_cleanup(self) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this function; it does too many things :)

  1. it verifies that we have keyword params after '*'; this should be done in state_parameter() (IIUC, we've got what we need to catch this there)
  2. it removes trailing whitespace from the param docstrings; this should be done in state_parameter_docstring()
  3. it reformats the docstring and assigns it to the function

IMO, only 3. should be part of the post-processing. Validation should be done by the state machine; that's one of the features of a well-behaving state machine :)

Copy link
Member Author

@AlexWaygood AlexWaygood Aug 1, 2023

Choose a reason for hiding this comment

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

You know where to file an issue about these problems ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

See gh-107550 for 2. Reworking 1. turned out to only make the code worse :(

"""
Called when processing the block is done.
"""
assert not line

if not self.function:
return

Expand Down