It should be possible to ignore "magic methods" like __str__ #60

Closed
Nurdok opened this Issue Dec 25, 2013 · 20 comments

Projects

None yet

8 participants

@Nurdok
Member
Nurdok commented Dec 25, 2013

There's no reason to force documentation for methods such as __str__, __unicode__, __repr__, etc.

@keleshev
Contributor

PEP 257 does not say explicitly what to do with magic methods. But is sais the follwoing:

Public methods (including the __init__ constructor) should also have docstrings.

Which makes me think that methods like __str__ should also be documented (they are public after all).

2 open questions:

  1. Is my interpretation correct?
  2. Should we allow disabling this with some command-line flag?
@sigmavirus24
Member

Is my interpretation correct?

I think so. All things considered, the magic methods work because they're technically public. No one will ever call: foo.__str__() explicitly but the public API is clearly str(foo). With that said, those methods can be considered private or implementation details. That brings up a different question for me: Should this consist of a whitelist? Consider someone using __method__ to hide an implementation detail. In that case, we shouldn't tell them they have to document it. (So I guess I answered my own question, but I'm not sure everyone agrees with me.)

@Nurdok
Member
Nurdok commented Dec 26, 2013

I think the correct interpretation of PEP257 is irrelevant. We should allow
to enforce what the user prefers.

If a user would add pep257.py to a build script or commit hook and expect
it to pass (i.e., you can't ignore errors), it should be possible to
configure the tool in the maximum possible granularity, otherwise no one
would use it.
On Dec 26, 2013 4:15 AM, "Ian Cordasco" notifications@github.com wrote:

Is my interpretation correct?

I think so. All things considered, the magic methods work because they're
technically public. No one will ever call: foo.str() explicitly but
the public API is clearly str(foo). With that said, those methods can be
considered private or implementation details. That brings up a different
question for me: Should this consist of a whitelist? Consider someone using
method to hide an implementation detail. In that case, we shouldn't
tell them they have to document it. (So I guess I answered my own question,
but I'm not sure everyone agrees with me.)


Reply to this email directly or view it on GitHubhttps://github.com/GreenSteam/pep257/issues/60#issuecomment-31209038
.

@keleshev
Contributor

I think the correct interpretation of PEP257 is irrelevant.

No, the correct interpretation of PEP 257 should be the default behavior.

We should allow to enforce what the user prefers.

Agree. This should be done through command-line flags, etc.

@Nurdok
Member
Nurdok commented Dec 26, 2013

I agree.
On Dec 26, 2013 12:33 PM, "Vladimir Keleshev" notifications@github.com
wrote:

I think the correct interpretation of PEP257 is irrelevant.

No, the correct interpretation of PEP 257 should be the default behavior.

We should allow to enforce what the user prefers.

Agree. This should be done through command-line flags, etc.


Reply to this email directly or view it on GitHubhttps://github.com/GreenSteam/pep257/issues/60#issuecomment-31217404
.

@adiroiban

__str__ , __repr__ , __eq__, __len__ or standard and a docstring would just by a dummy repetive text.

Same for

@propery
def name(self):
    """Docstring here."""

@getter.setter
def name(self, value):
    """Dumb docstring as this will alwasy be a setter for name."""

I think that developers should spend time write meaningful docstring rather than add dumb docstring to please a tool.

@merwok
merwok commented Aug 6, 2014

I agree that magic methods don’t need docstrings (the language defines their interface and role), with a possible exception of init. I tend to document constructors in the class docstring, but sometimes it makes more sense to add a docstring to init.

@jacebrowning
Contributor

I agree with @merwok and @adiroiban. The behavior of most magic methods should be completely obvious based on the rest of the class, but __init__ is necessarily specific to the class and its docstring should explain the purpose of the constructor's arguments.

@mths0x5f

And so...? Is now possible disable this with a command-flag or nah?

@Nurdok
Member
Nurdok commented Oct 17, 2014

Not as of now.
On Oct 17, 2014 3:26 AM, "Matheus" notifications@github.com wrote:

And so...? Is now possible disable this with a command-flag or nah?


Reply to this email directly or view it on GitHub
#60 (comment).

@Nurdok Nurdok self-assigned this Aug 1, 2015
@Nurdok Nurdok modified the milestone: Next release Aug 2, 2015
@Nurdok
Member
Nurdok commented Sep 23, 2015

I was thinking about how to implement this. If possible, I would like all customization for errors to happen by ignoring and selecting errors, so naturally I would like to have a new error code for magic methods
However, there is the issue of the exceptions that probably should be documented. We talked about __init__, but there maybe some others in that group - e.g., __call__.

Some questions:

  1. should the new error code exclude these "special" magic methods?
  2. Is it clear which magic methods are "special" in this way?
@jacebrowning
Contributor

Is it clear which magic methods are "special" in this way?

I would say any magic method that takes a variable number of arguments should be documented. The arguments to (and behavior of) methods like __str__, __add__, __setattr__, etc. are well-defined in the data model: https://docs.python.org/3/reference/datamodel.html

@Nurdok
Member
Nurdok commented Sep 23, 2015

From what I see, the only magic methods with a variable number of arguments are __init__, __new__ and __call__. So I'm currently leaning towards creating a new "missing docstring in magic method" error that excludes just these three methods. Any objections?

@Nurdok
Member
Nurdok commented Sep 23, 2015

See a summary of magic methods here: http://www.rafekettler.com/magicmethods.html#appendix1

@jacebrowning
Contributor

👍

@Nurdok Nurdok closed this Oct 9, 2015
@jab
jab commented Feb 15, 2016

I'm running the latest release (1.0), which it looks like this landed in, but I'm finding I have to explicitly pass --add-ignore=D105 to suppress complaints about missing docstrings in my __str__ methods. It looks like the intention of this issue was to suppress these by default. Did I miss something or misinterpret?

@Nurdok
Member
Nurdok commented Feb 15, 2016

@jab The new error is not suppressed by default. It exists so that you could suppress it, if you want to.

@jab
jab commented Feb 15, 2016

@Nurdok Did you mean not suppressed by default?

@Nurdok
Member
Nurdok commented Feb 15, 2016

@jab yes, edited.

@jab
jab commented Feb 15, 2016

Thanks for clarifying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment