Skip to content

[docutils]: Add annotations for docutils.parsers.rst.states#14209

Open
Harry-Lees wants to merge 17 commits into
python:mainfrom
Harry-Lees:14031-patch-1
Open

[docutils]: Add annotations for docutils.parsers.rst.states#14209
Harry-Lees wants to merge 17 commits into
python:mainfrom
Harry-Lees:14031-patch-1

Conversation

@Harry-Lees

@Harry-Lees Harry-Lees commented Jun 1, 2025

Copy link
Copy Markdown
Contributor

Linked Issue: #14031

This PR overlaps with #14191, apologies to @donbarbos, I didn't see your PR while working on this. I think this PR is a more fully implemented version of annotations in docutils.parsers.rst.states, but #14191 covers significantly more than just this file.

It might be possible to merge the two together, I will check the other PR to ensure that they line up on the parts which are overlapping.

Edit: I merged some changes where I thought #14191's implementation was more complete than this one, I added Co-authored attribution to the relevant commits, @donbarbos if you are unhappy in any way with the attribution, please let me know how I can update it, I'm not too well versed in Git, but I think I can transfer the commit ownership entirely, or remove you from the commit(s) entirely if you're unhappy to have your name associated with them :)

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@donbarbos

Copy link
Copy Markdown
Contributor

Thank you, I suggest merging my PR first and then we can see what improvements you have made ;)

I will also note that stubs for docutils are already fully generated in my draft PR (WIP) and for now I am sorting out the errors and sending separate updates for directories

@srittau

srittau commented Jun 3, 2025

Copy link
Copy Markdown
Collaborator

I've merged @donbarbos's PR. This now has (as expected) merge conflicts.

@Harry-Lees

Copy link
Copy Markdown
Contributor Author

I rebased the PR and fixed a couple of obvious errors, I'm sure the CI will have some failures as-well :D.

I need to take a more detailed look at some of the differences, I should be able to get round to it later today.

Comment thread stubs/docutils/docutils/parsers/rst/states.pyi Outdated
self, match: Match[str], context: list[str] | None, next_state: str | None
) -> tuple[list[str], str | None, list[str]]: ...
self, match: Match[str], context: Any, next_state: _NextState
) -> tuple[list[Any], _NextState, list[Any]]: ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would appreciate some input in these cases as-well. The implementing function is:

def field_marker(self, match, context, next_state):
    """Field list field."""
    field, blank_finish = self.field(match)
    self.parent += field
    self.blank_finish = blank_finish
    return [], next_state, []

I'm not sure how the [] should be annotated ideally. This is another very common pattern which I have always annotated as list[Any]. I stayed away from list[Never] here, because this will throw an error if the array is appended to (which I can only assume it often is)

@AA-Turner AA-Turner Jun 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You will find answers in the docstring of the State class:

    Transition methods all return a 3-tuple:

    - A context object, as (potentially) modified by the transition method.
    - The next state name (a return value of ``None`` means no state change).
    - The processing result, a list, which is accumulated by the state
      machine.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

github-actions Bot commented Jul 7, 2025

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@srittau srittau left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm sorry for the late review.

This looks mostly good, but I noticed that some context arguments are annotated with Any. We generally need comments explaining non-obvious uses of Any. But I also noticed that there already is a _Context type var. Could this be used? If not, I would recommend renaming that type var (maybe to _ContextT) and instead introduce a _Context type alias (potentially to Any) and add any explanatory comment to the type var definition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants