Skip to content

Conversation

@smaye81
Copy link
Contributor

@smaye81 smaye81 commented Mar 27, 2025

Here is a proposal for how to structure docstrings in protovalidate-python going forward (at least when porting the rest of the validation implementations). I think this strikes the right balance of adhering to best practices while not being onerous.

  • Functions being added to the library should get the bulk of the comment description in a docstring. No need to define Args:, Raises:, etc. though.
  • Any other public function, class, etc. should preferably have docstrings, though not necessary.
  • Be as succinct as possible while also keeping comments across implementations consistently worded.

For validation following an RFC / spec:

  • Always include any relevant ABNF grammar and keep aligned.
  • Indent grammar inside docstring.

One suggestion outside the scope of this PR might be to always have full docstring (Args, Raises, etc.) on any public method the user interacts with (i.e. stuff in validator.py)

@smaye81 smaye81 changed the title Docstring style proposal Propose docstring style for repo Mar 27, 2025
@smaye81 smaye81 changed the base branch from main to sayers/validation_upgrades March 27, 2025 01:16
@smaye81 smaye81 requested review from rodaine and timostamm March 27, 2025 01:18
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

👍

@smaye81
Copy link
Contributor Author

smaye81 commented Mar 27, 2025

@timostamm made even more consistent with other implementation comments here: 001449d.

is_uri_reference wouldn't pass a linter, but we can worry about that if we ever turn one on for docstrings.

@smaye81 smaye81 merged commit bb6012d into sayers/validation_upgrades Mar 27, 2025
7 checks passed
@smaye81 smaye81 deleted the sayers/docstrings branch March 27, 2025 12:52
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.

3 participants