Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.9k
[docutils]: Add annotations fordocutils.parsers.rst.states#14209
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 |
I've merged@donBarbos's PR. This now has (as expected) merge conflicts. |
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. |
Uh oh!
There was an error while loading.Please reload this page.
| 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]]: ... |
There was a problem hiding this comment.
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:
deffield_marker(self,match,context,next_state):"""Field list field."""field,blank_finish=self.field(match)self.parent+=fieldself.blank_finish=blank_finishreturn [],next_state, []
I'm not sure how the[] should be annotated ideally. This is another very common pattern which I have always annotated aslist[Any]. I stayed away fromlist[Never] here, because this will throw an error if the array is appended to (which I can only assume it often is)
There was a problem hiding this comment.
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 theState 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. This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
According tomypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
There was a problem hiding this comment.
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 somecontext arguments are annotated withAny. We generally need comments explaining non-obvious uses ofAny. 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 toAny) and add any explanatory comment to the type var definition.
Uh oh!
There was an error while loading.Please reload this page.
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 :)