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

Updated guidelines for single line docstrings #999

Closed
wants to merge 1 commit into from

Conversation

clintonb
Copy link
Contributor

The guideline now aligns with PEP 257. There is no obvious reason for deviating.

The guideline now aligns with PEP 257. There is no obvious reason for deviating.
@rlucioni
Copy link

👍

1 similar comment
@pdesjardins
Copy link
Contributor

👍

@dan-f
Copy link
Contributor

dan-f commented Apr 28, 2016

👍 - could we specify whether or not we allow 'buffer' spaces? e.g. """A single-line docstring""" vs """ A single-line docstring """?

My preference is the first option, since it follows how we don't pad function calls/dicts/etc with spaces.

@rlucioni
Copy link

I agree with @dan-f, my preference is no whitespace.

@doctoryes
Copy link
Contributor

I prefer consistency between one- and multi-line comments so 👎 . But - the PEP is stacked against me and I'm flexible.

@stvstnfrd
Copy link
Contributor

stvstnfrd commented Apr 28, 2016

Redacted: ignore: premature submission

@stvstnfrd
Copy link
Contributor

I'm with @doctoryes for consistency.
By consistently using multiline docstring, you reduce the size of the
diff, in the event that documentation is ever expanded [1].

I doubt you guys worry about how code diffs in the same way we do;
it's a constant pain living down-wind from edX.

Imagine a code block like:

def get_users():
    """
    Get teh users
    """

But then at our fork, we need to explain additional
assumptions/considerations, so we augment the docs:

def get_users():
    """
    Get teh users

    At Stanford, we must also filter out all DirectAccess users to
    exclude anonymous access from the reports.
    """

If everyone always uses multiline, the diff is clean and less likely to
conflict.

def get_users():
    """
    Get teh users
+
+     At Stanford, we must also filter out all DirectAccess users to
+     exclude anonymous access from the reports.
    """

So if upstream eventually decides to fix the initial typo and/or add
more context, we won't have conflicts when we merge.

def get_users():
    """
-     Get teh users
+     Get the users

    At Stanford, we must also filter out all DirectAccess users to
    exclude anonymous access from the reports.
    """

On the other hand, if we need to edit your single-line docstring, we're
guaranteed to have conflicts if we both edit the docs.

def get_users():
<<<<<<< US
    Get teh users

    At Stanford, we must also filter out all DirectAccess users to
    exclude anonymous access from the reports.
    """
=======
    """Get the users"""
>>>>>>> THEM

You may consider that trivial, but when we're resolving dozens and
dozens of those every time we merge, it quickly becomes a real pain.
And of course, the more conflicts we have to resolve, the more chance
there is we'll mistakenly introduce bugs during conflict resolution;
we've been bitten by this before.

Perhaps there's a larger discussion to be had w/r/t "the style of the
diff", but many of the ways in which edX writes code makes it more
difficult than it has to be to integrate forked code. Given that, while
I consider PEP8/257/etc. to be a good baseline, I would like to see us
enforce stricter guidelines when/where it makes the code easier to work
with.

[1] Everyone is always positive that this one-liner will always just
be a single line... that often doesn't hold.

@dan-f
Copy link
Contributor

dan-f commented Apr 28, 2016

Thats a super useful (and well written!) example @stvstnfrd. I think we should prioritize our community's ability to work off of our platform above a PEP.

@nasthagiri
Copy link
Contributor

@clintonb I second the comment from @stvstnfrd and prefer keeping with the multi-line comment standard as it is best for code reviews and diffs. As an open source platform, I agree that we should weigh the convenience of code reviewers over PEP8 standards.
So 👎 .

@clintonb clintonb closed this Apr 29, 2016
@clintonb clintonb deleted the clintonb/single-line-docstring branch April 29, 2016 02:27
@dan-f
Copy link
Contributor

dan-f commented Apr 29, 2016

Is there any interest in updating the docs to explicitly call out the issue that @stvstnfrd keeps on running into? It seems as though people are ignoring this documentation, and it might be helpful motivation.

I can do it if there's sufficient interest.

@rlucioni
Copy link

@dan-f I think documenting the issue @stvstnfrd explained would be a great idea. If we're going to deviate from PEP 257, it would be good to explain why.

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.

7 participants