fix: convert remaining rst docstrings#1244
Conversation
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
AngeloDanducci
requested review from
a team,
jakelorocco and
nrfulton
as code owners
planetf1
left a comment
planetf1
left a comment
There was a problem hiding this comment.
A few gaps in the RST conversion — details inline.
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
planetf1
left a comment
planetf1
left a comment
There was a problem hiding this comment.
Thanks for the follow-up commit — the issues I flagged in the first round are all resolved: the Example: and Deprecated: fences in interpreter.py are in place, the CONTRIBUTING.md fence nesting is correct (4-backtick outer, 3-backtick inner), and the documents type annotation in intrinsic/core.py is now present. Approving with a few small suggestions below.
| # Pattern for ``python ... `` blocks | ||
| python_blocks = re.findall(r"``python\s*\n(.*?)\n``", content, re.DOTALL) | ||
| # Pattern for ``python / ```python blocks (RST and Markdown) | ||
| python_blocks = re.findall(r"```?python\s*\n(.*?)\n```?", content, re.DOTALL) |
There was a problem hiding this comment.
Nice catch making the fences RST-aware. One gap: the optional third backtick means we now also match python `` blocks, but all the existing tests in test_reqlib_python.py use triple-backtick Markdown — which the old regex already matched. So the new double-backtick path has no coverage. Worth adding a test case that feeds in something like:
``python
def f():
return 1
``
...and asserts the block is extracted. That locks in the behaviour this change is actually for.
| Args: | ||
| response (str | None): Assistant response. When `None`, extracted from the | ||
| last assistant output in `context`. | ||
| documents (collections.abc.Iterable[str | Document]): Documents used to generate `response`. Each element may be a |
There was a problem hiding this comment.
Cosmetic only (ruff doesn't flag docstring prose), but this line runs long with the fully-qualified type inline. Breaking after the type keeps it aligned with the other args.
| documents (collections.abc.Iterable[str | Document]): Documents used to generate `response`. Each element may be a | |
| documents (collections.abc.Iterable[str | Document]): Documents used to | |
| generate `response`. Each element may be a |
| """Separate the system message from other messages. | ||
|
|
||
| :returns: Tuple of system message, if present, and remaining messages. | ||
| Returns: |
There was a problem hiding this comment.
Not a regression — the old :returns: was just as vague — but since we're touching this, the signature gives us the element types and the conversion was a chance to make the Returns line precise.
| Returns: | |
| tuple[SystemMessage | None, list]: The system message if present | |
| (else `None`), and the remaining non-system messages. |
| Note: Prefer `log_context` as the primary API — it guarantees cleanup | ||
| (including restoring outer values on same-key nesting) even on exceptions. |
There was a problem hiding this comment.
Tiny style nit while we're tidying these — Google style puts Note: on its own line with the body indented under it, matching how Args:/Raises: render. Inline works but is slightly inconsistent.
| Note: Prefer `log_context` as the primary API — it guarantees cleanup | |
| (including restoring outer values on same-key nesting) even on exceptions. | |
| Note: | |
| Prefer `log_context` as the primary API — it guarantees cleanup | |
| (including restoring outer values on same-key nesting) even on exceptions. |
markstur
left a comment
markstur
left a comment
There was a problem hiding this comment.
some args are lost when converting from init doc to class docs.
Also Nigel already pointed out a Note indent problem.
| :param input_path_expr: Path expression for the list of records to explode | ||
| :param target: Name of list-valued field within each record. | ||
| """ | ||
| """Initialize Explode transformation rule.""" |
There was a problem hiding this comment.
To me, it looks like some param docstrings were lost. I couldn't figure out if it was intentional. With AI help here is the summary:
The PR is following these new rules by removing :param: from init docstrings. But it's incomplete - it removed the RST :param: entries but didn't add the Google-style Args: sections to the class docstrings.
Summary of the Issue
The PR correctly identifies that RST :param: syntax needs to be removed, but fails to complete the conversion for 4 classes in mellea/formatters/granite/intrinsics/output.py:
Classes missing Args: sections after the PR:
- Explode
- DropDuplicates
- Project
- Nest
These classes had :param: entries in their init docstrings (which the PR removes), but the PR doesn't add equivalent Args: sections to their class docstrings. This leaves their constructor parameters undocumented.
The fix needed: Add Args: sections to these 4 class docstrings documenting their init parameters, following the pattern already established by MergeSpans and DecodeSentences in the same file.
More specifics:
The PR removes RST :param: entries from __init__ docstrings but does not add Google-style Args: sections to the class docstrings. This leaves 4 parameters undocumented — one per subclass.
Undocumented Parameters
| Class | Line | Parameter | Current :param: (Being Removed) |
|---|---|---|---|
Explode |
746 | target_field |
:param target: Name of list-valued field within each record. |
DropDuplicates |
814–815 | target_fields |
:param target_fields: Names of fields to use for determining whether two records are duplicates. |
Project |
872–874 | retained_fields |
:param retained_fields: Names of fields that remain after the projection. Can be either a list of fields or a mapping from retained field to new name of retained field. |
Nest |
922–923 | field_name |
:param field_name: name of the single field in the JSON object that this rule will wrap around each matching value. |
Explode: The :param name is target but the actual parameter is target_field — a pre-existing typo.
3 participants
Pull Request
Issue
Fixes #1172
Description
Converts remaining RST docstrings
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.