#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:
---------------------------------------------------+------------------------
Changes (by mguaypaq):
* status: needs_info => needs_review
Comment:
Replying to [comment:3 ncohen]:
> I never used this thing, and to me a code which is not documented might
as well not be there, if it is only meant to be used by the guy who added
it in the first place.
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.
> This being said, has it become illegal to use list as an object's
attribute ? I understand that you want an immutable version of everything
that is in Sage, but if it means preventing people from writing code as
they want to perhaps you should change your plans `O_o`
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?
> By the way, who "frowns upon" mutable arguments ? It's useful to have a
list somewhere, from time to time `O_o`
The problem is only with mutable _default_ arguments. Go ahead and pass a
list when you call a function whenever you want.
Replying to [comment:4 ncohen]:
> The patch looks good to me, but I don't get why you simultaneously :
> * Fix the docstring to say that if the input it an empty list (instead
of None, formerly) then the digraph is considered to be a digraph without
boundary
> * Give a meaning to a None value, which is exactly what the
documentation described in the first place
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).
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.
Replying to [comment:7 ncohen]:
> (and you probably removed a ' by mistake at the end of a line)
Yes, good catch, sorry about that. The updated patch should fix this.
Cheers,
Mathieu
Apply trac_14794_v1.2.patch
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14794#comment:8>
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.
