#14794: DiGraph constructor doc describes `boundary` option wrong
---------------------------------------------------+------------------------
Reporter: mguaypaq | Owner: mguaypaq
Type: defect | Status:
needs_review
Priority: major | Milestone: sage-5.12
Component: graph theory | Resolution:
Keywords: graph, digraph, mutable default | Work issues:
Report Upstream: N/A | Reviewers:
Authors: Mathieu Guay-Paquet | Merged in:
Dependencies: | Stopgaps:
---------------------------------------------------+------------------------
Comment (by ncohen):
> I'm not the one who added it, but it did cause a functional problem (did
you see the failing test in the ticket description?), namely the `show()`
method blowing up.
Yeahyeah of course. I don't like this code either. Not documented, don't
know what it does. Actually I would like to have a non-autoritary way to
get rid of it. Otherwise we'll jkust be shipping useless codes forever.
> I know I got a previous patch rejected for using a list instead of a
tuple or `None` as a default argument before, and I now agree with this
policy (which is part of PEP 8). Why potentially shoot yourself in the
foot for no benefit at all?
What exactly is the problem with default mutable arguments ? PEP8 even
tells me that I should not put blank lines in my code when I think that it
improves readability (cf #14562), so to me PEP8 is like the new version of
my high school internal rules.
> It's true, I'm fixing the problem in two different ways. Maybe this is
overkill. Still, bringing the documentation of `DiGraph` in line with the
documentation of `Graph` seemed like a good idea. Another fix would be to
use `()` instead of `None` as the default argument, and then either turn
it into a list later (or change the rest of the code to deal with
`_boundary` begin a tuple, which I volunteer to do if no one objects).
Oh. I see. It's just that as it is, you set the default value to something
that the documentation does not claim to be admissible.
> I'm happy with any solution where:
> * the attribute `_boundary` is always an iterable, and not `None`,
since the rest of the code expects this, and
> * the `__init__` method does not have mutable default arguments.
>
> If you have more/different requirements, let me know.
I don't have any requirements. Well, short of understanding why mutable
default arguments are now against the Dogma `O_o`
Nathann
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14794#comment:9>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/groups/opt_out.
