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

Revert "Convert docstrings to reStructuredText" #246

Merged
merged 1 commit into from
Mar 3, 2018

Conversation

RecursiveForest
Copy link
Contributor

@RecursiveForest RecursiveForest commented Mar 2, 2018

This reverts commit 3b1bd24.

I'm not sure what the intended goal of this change was. Almost all of our docstrings are internal developer documentation and should stay as lean as possible to not waste screen space. I don't see any reason to bring back the old morituri-style bloat of useless and unnecessary parameter lists, especially when their "type" field is often incorrectly the same string as the parameter value.

Joe, please follow convention and create pull requests for your own changes, especially large style-changing ones like this, and give us a chance to review them, instead of essentially merging them directly to master with a merge bubble.

@MerlijnWajer
Copy link
Collaborator

I don't mind making rst comments multiline. I have to look up what the right way to define parameters in function comments is, but I think that's also fine. The weird "I (as a function) do this" can go, though.

I'm assuming this was done in part to please some tools that complain about missing doc?

@RecursiveForest
Copy link
Contributor Author

I don't mind multiline docstrings, the param/type lists just provide almost no value. I feel very strongly about this.

@RecursiveForest
Copy link
Contributor Author

I should also state that I don't mind the whitespace and formatting changes that were part of this PR-- those were all good (although I'd advise creating them as a separate PR to make tracking changes easier). I don't even mind using rST for parts of our docstrings (like spelling code snippets with backticks, etc), but I do mind adding boilerplate parameter lists to function docstrings.

@JoeLametta
Copy link
Collaborator

Fine, let's merge this then rework that commit.

@RecursiveForest
Copy link
Contributor Author

Okay, I'll resolve the new conflicts when I get home.

@RecursiveForest
Copy link
Contributor Author

Resolved.

@JoeLametta JoeLametta merged commit 7b5af61 into whipper-team:master Mar 3, 2018
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