docs: convert Component._encode docstring to Google style - #1617
docs: convert Component._encode docstring to Google style#1617harsh8968 wants to merge 1 commit into
Conversation
Contributes to collective#1072. Replaces the Sphinx-style :param:/:type:/:returns: markup on Component._encode with the project's Google-style Parameters/Returns sections, per the documentation style guide. No behavior change. AI disclosure: I used Claude (Anthropic, Sonnet 5) to identify an unclaimed target for icalendar issue collective#1072, draft the converted docstring text following the project's style guide, and verify it locally (ruff, pytest, doctest). I reviewed and validated the change.
|
This pull request did not pass quality checks and AI use is suspected. Please review Contribute and make any necessary amendments. |
|
Profile summary: Full profile |
stevepiercy
left a comment
There was a problem hiding this comment.
This is a good start. I realized I omitted a critical piece, specifically that the type of the parameter must be specified, either in the signature's type hints or in the parameter's docstring, with type hints preferred. I also suggested to change the booleans to inline literals. Would you please take care? Thank you!
| :type encode: Boolean | ||
| Parameters: | ||
| name: Name of the property. | ||
| value: Value of the property. Either a basic Python type or |
There was a problem hiding this comment.
| value: Value of the property. Either a basic Python type or | |
| value: Value of the property. Either a native Python type or |
| @@ -248,23 +248,18 @@ def is_empty(self) -> bool: | |||
| def _encode(cls, name, value, parameters=None, encode=1): | |||
There was a problem hiding this comment.
When converting to Google style, you must either add type hints in the signature or name the parameter's type. Type hints are preferred. Sorry that wasn't clear in the requirements.
It would be good to include that in both the documentation and original issue. I'll handle both of these.
There was a problem hiding this comment.
The signature has no type hints and the docstring does not name the types for any parameter. The style guide (updated since this PR opened) requires one or the other.
Since _encode is a private method and encode defaults to 1 rather than True, adding type hints to the signature risks a misleading annotation.
Adding inline types in the docstring is safer here; see the style guide's type hints section for the format.
| value: Value of the property. Either a basic Python type or | ||
| any of icalendar's own property types. | ||
| parameters: Property parameter dictionary for the value. Only | ||
| used if encode is set to True. |
There was a problem hiding this comment.
The original should have used inline literals.
| used if encode is set to True. | |
| used if encode is set to ``True``. |
| encode: True if the value should be encoded to one of | ||
| icalendar's own property types (falls back to ``vText``), | ||
| or False if not. |
There was a problem hiding this comment.
| encode: True if the value should be encoded to one of | |
| icalendar's own property types (falls back to ``vText``), | |
| or False if not. | |
| encode: ``True`` if the value should be encoded to one of | |
| icalendar's own property types (falls back to ``vText``), | |
| or ``False`` if not. |
| @@ -0,0 +1 @@ | |||
| Converted the ``Component._encode`` docstring from Sphinx ``:param:``/``:type:``/``:returns:`` markup to the project's Google-style ``Parameters``/``Returns`` format. I used AI (Claude) to help draft the docstring and this entry. @harsh8968 | |||
There was a problem hiding this comment.
Use the full path for methods that don't render in the documentation. Also the minutia of how it was changed are unnecessary.
| Converted the ``Component._encode`` docstring from Sphinx ``:param:``/``:type:``/``:returns:`` markup to the project's Google-style ``Parameters``/``Returns`` format. I used AI (Claude) to help draft the docstring and this entry. @harsh8968 | |
| Converted the ``icalendar.cal.component.Component._encode`` docstring from the default Sphinx markup to the project's Google-style format. I used AI (Claude) to help draft the docstring and this entry. @harsh8968 |
SashankBhamidi
left a comment
There was a problem hiding this comment.
Thanks for the conversion.
Steve's suggestions are all still open, please apply them before this merges.
@stevepiercy Do you want to batch add them and merge? Since there's not response from the author for over a week.
| @@ -248,23 +248,18 @@ def is_empty(self) -> bool: | |||
| def _encode(cls, name, value, parameters=None, encode=1): | |||
There was a problem hiding this comment.
The signature has no type hints and the docstring does not name the types for any parameter. The style guide (updated since this PR opened) requires one or the other.
Since _encode is a private method and encode defaults to 1 rather than True, adding type hints to the signature risks a misleading annotation.
Adding inline types in the docstring is safer here; see the style guide's type hints section for the format.
This PR was created 6 days ago. Especially during summer and winter when people go on extended vacations, I'm inclined to let contributors have time away for life, and allow at least two weeks for them to respond to feedback. After that period elapses, I'd post another comment asking them if they need anything from us to address the feedback, then wait at least two more weeks before taking it over. That's a general rule, but exceptions may apply, such as if it's a high priority contribution. Docstring conversions, although important, are a low priority. |
Linked issue
Description
Converts the
Component._encodedocstring insrc/icalendar/cal/component.pyfrom the old Sphinx:param:/:type:/:returns:markup to the project's Google-styleParameters/Returnsformat, per the style guide. Docstring only, no behavior change.Checklist
Additional information
Verified with
ruff check/ruff format --check, thetest_docstring_of_python_file[icalendar.cal.component]doctest, and the full component/encode-related test selection — all pass. The unrelatedtest_timezone_identification.py/test_issue_722_generate_vtimezone.pyfailures in the full suite are a pre-existing dateutil/tzdata environment issue on Windows, reproducible on a cleanmaincheckout without this change.I'm a first-time contributor and used Claude (Anthropic) to help find an unclaimed part of #1072 and draft the docstring per the style guide; disclosed per the AI policy.