Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(python): ensure Raises: section is after Returns: section in google style #194

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

hassec
Copy link
Contributor

@hassec hassec commented Aug 5, 2024

Changes google_docstrings.lua template to ensure the Raises: section is after the Returns: section, as is shown in the Google Style Guide

Closes #193

@ColinKennedy
Copy link
Contributor

I don't see anything in Google's styleguide saying that Raises is meant to come beforehand, just a couple examples where it does. Luckily Google does have a way to seek clarification. e.g. in the past I asked + PRed to google/styleguide#505

We've got a custom pylint GoogleDocstringChecker internally that verifies the our style of docstring format. My internal response to this kind of question is generally to defer to whatever our lint checker implementation asks for when it comes to details like this. Running your final example through that, it likes it. Which is a strong indicator that we allow that nice readable form. :)

It'd be good to get confirmation before moving forward with this PR.

@hassec
Copy link
Contributor Author

hassec commented Aug 9, 2024

@ColinKennedy let's see if someone from Google replies. Otherwise, I'd argue that w/o guidance, following the Google examples would be the more reasonable default, or make it configurable.

@hassec
Copy link
Contributor Author

hassec commented Dec 3, 2024

@ColinKennedy @danymat, now that we've got a response on the google issue, how do you feel about moving forward?

@ColinKennedy
Copy link
Contributor

Yes that's fine by me. Though I would have rather Google's style guide explicitly say it in the text itself. The ultimate test, I've found, is to just submit a PR that directly edits the style guide with your suggested wording and see if they merge it.

Though I personally really think it's a mistake to put Raises: after Returns:. After all, raising always interrupts before a regular return so it makes sense to have it come before it in the docs. And Args: of course is the entry point to a function. So it flows Args:, Returns: or Args:, (intteruption), Raises:, Returns: But that's just my opinion.

Anyway, I'm not sure if @danymat still maintains this repo, I don't have write access. You still out there buddy?

@danymat
Copy link
Owner

danymat commented Dec 4, 2024

Anyway, I'm not sure if @danymat still maintains this repo, I don't have write access. You still out there buddy?

Indeed, maintaining it is becoming quite hard due to my new life as security researcher
@ColinKennedy, if you are interested, I could grant you write access to the repository in a first time, to ease the PR processes

Related to this fix, as I read in the mentioned PR from google style guide, we got a suitable response. I will then proceed with the changes; remember that you can still reorder the template by yourself on your config

@danymat danymat merged commit 37dd095 into danymat:main Dec 4, 2024
1 of 4 checks passed
@ColinKennedy
Copy link
Contributor

@danymat yes I'm interested! If you want chat about any details related to what you're comfortable with me approving or merging, my email is in my profile

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.

Python: Should Raises be after Return in google style?
3 participants