__all__ can be a tuple or a list

[smoyerx]

Parse_all() flags an error if all is not a tuple, whereas all is more commonly a list.

[keleshev]

This is because if __all__ is mutable, then pep257 can't really tell if it was modified or not. So I decided to stick to tuples. Didn't think it would be a problem. Is it?

[smoyerx]

Most of the code I've seen makes __all__ a list, as do all of the examples in the official Python documentation. PEP 8 also refers to __all__ as a list. And there may be other common tools that rely on this convention (although that would technically be incorrect).

Also making __all__ a tuple doesn't completely address the problem you're trying to solve. While the tuple is immutable, the code can change the binding of __all__ to reference a different tuple.

Perhaps a good approach is to skip the __all__ validation if it is a list, or make it a command line option to do so.

[keleshev]

Also making __all__ a tuple doesn't completely address the problem you're trying to solve. While the tuple is immutable, the code can change the binding of __all__ to reference a different tuple.

__all__ could be only re-bound at the top level. pep257 will recognise that.

Perhaps a good approach is to skip the __all__ validation if it is a list, or make it a command line option to do so.

That would defeat the purpose of having __all__. pep257 would not know what's public and what's not.

[smoyerx]

I'm in complete agreement that making __all__ a tuple is a win for static checking. My only point in creating this issue was to point out that in the bulk of python code __all__ is a list so most users of pep257 will see this error message.

That said I'll close this issue. Perhaps generating this message will provide incentive to switch to using tuples to get the benefit of __all__ checking.

BTW thanks for creating this handy tool -- I run all my python code through it to keep myself consistent.

[ziadsawalha]

Is there a need for pep257 to know what is public or not? Are there different PEP-257 rules for public and private things?

[keleshev]

@ziadsawalha PEP 257 says:

...and all functions and classes exported by a module should also have docstrings.

To enforce that pep257 should know which functions and classes are private and which are public. If there is no __all__ it is simple—things with leading underscore are private, otherwise—public. With __all__ it is slightly more difficult. If __all__ is mutable, then there is no way to determine it contents statically. Maybe the module modifies __all__ based on current temperature in Rome. However, if it is immutable (tuple), we can reason about what is public and what is private statically.

[keleshev]

So pep257 checks that __all__ is a tuple not for its own sake, but to give warnings about public functions which don't have docstrings.

[keleshev]

It's probably best to document this behavior either in README or in the error-message, though.

[merwok]

FWIW I’ve seen __all__ being a list 99% of the time. I think I remember a Python bug where inspect or pydoc choked on a tuple __all__, which shows that Python core developers always thought it would be a list.

[ziadsawalha]

@halst Could you help me find a test or condition under which #63 will fail? I'll be happy to modify the code around that.

[keleshev]

__all__ = ['g']

def f():
    pass

def g():
    """Docstring."""

__all__.append('f')

In this case, even though f is in __all__, pep257 will not be able to warn that f is missing a docstring, because it is public.

[ziadsawalha]

Yeah..... hmmmm

But that's almost impossible to get water-tight. What if you have something like this:

import os

__all__ = ['g']

def f():
    pass

def g():
    """Docstring."""

if os.environ.get("FEATURE_F"):
    __all__.append('f')

I feel like I've come across many more projects that have all as a static list than I have come across ones where it is dynamically changed; but that might be the types of projects I work with. I don't have solid data on that and I'm not sure how to get it.

How about if we add an option (ex. --all-as-list) that made pep257 not break on all as a list?

[keleshev]

I would rather have no option, but a warning if __all__ is a list. Something along the lines of:

./file.py: WARNING: __all__ is defined as a list, this means pep257 cannot reliably detect contents of __all__ variable, because it can be mutated. Change __all__ to be an (immutable) tuple, to remove this warning. Note, pep257 is using __all__ to detect which definitions are public, to warn if public definitions are missing docstrings. If __all__ is a (mutable) list, pep257 cannot reliably assume its contents. Now pep257 will proceed assuming __all__is not mutated.

Not sure that this is the best wording, though. Anyway, pep257 will proceed and will return 0 code in no other errors were found—so it's a pure warning that will not influence success/failure of the static analysis.

[ziadsawalha]

@halst How about this? I added the following to #63:

        if self.current.value == '[':
            msg = ("%s WARNING: __all__ is defined as a list, this means "
                   "pep257 cannot reliably detect contents of the __all__ "
                   "variable, because it can be mutated. Change __all__ to be "
                   "an (immutable) tuple, to remove this warning. Note, "
                   "pep257 uses __all__ to detect which definitions are "
                   "public, to warn if public definitions are missing "
                   "docstrings. If __all__ is a (mutable) list, pep257 cannot "
                   "reliably assume its contents. pep257 will proceed "
                   "assuming __all__ is not mutated.\n" % self.filename)
            sys.stderr.write(msg)

And the output looks like this:

$pep257 pep257.py | grep WARN
pep257.py WARNING: __all__ is defined as a list, this means pep257 cannot reliably detect
contents of the __all__ variable, because it can be mutated. Change __all__ to be an (immutable)
tuple, to remove this warning. Note, pep257 uses __all__ to detect which definitions are public, to
warn if public definitions are missing docstrings. If __all__ is a (mutable) list, pep257 cannot
reliably assume its contents. pep257 will proceed assuming __all__ is not mutated.

[ziadsawalha]

Thanks, @halst. @smoyergh - this is now addressed in #63 and merged in.

[ssbarnea]

Is there a way to disable these really annoying warnings? We have some legacy code that we don't want to update but we still have to maintain it for a while.

Why would we have a warning that cannot be disabled using the normal ignore mechanism?

[ask]

I'm not sure I see the point of this warning, as even __all__ as a tuple can be modified:

import os

__all__ = ('g',)

def f():
    pass


def g():
    """Docstring."""


if os.environ.get('FEATURE_F'):
    __all__ += ('f',)

This makes __all__ point to a new tuple object in memory, so it's not actually mutable, but both lists and tuples have the same problem when statically parsing a module.

[jdufresne]

The Python official docs refer to __all__ as a list and write all examples using a list. This warning is contradicting the language's own documentation. Further, as @ask has shown, this offers no real protection against mutating the __all__ reference.

IMO, I think you should consider removing thewarning and instead document pydocstyle's shortfall when handling modules that mutate __all__.