#19936: Make num_faces
-------------------------------------+-------------------------------------
Reporter: kcrisman | Owner:
Type: enhancement | Status: needs_review
Priority: minor | Milestone: sage-7.1
Component: graph theory | Resolution:
Keywords: | Merged in:
Authors: Marco Cognetta | Reviewers:
Report Upstream: N/A | Work issues:
Branch: | Commit:
u/mcognetta/make_num_faces | 10c32d2bf9f707f7965a2599c66c59c093d17108
Dependencies: | Stopgaps:
-------------------------------------+-------------------------------------
Comment (by mcognetta):
Replying to [comment:3 tscrim]:
> Thanks for making this contribution. Here are some comments on what you
will need to do in order to get this into Sage:
>
> - It's a good idea to not remove the raw string formatting (i.e., the
`r` before a string) unless there is a specific reason to do so. Even more
so when it is in a part unrelated to the ticket.
> - Similarly, it's not a good idea to add whitespace in a place unrelated
to the ticket.
> - You should try to conform to PEP8 as much as possible.
> - You need to also make the one-line documentation a declarative
statement.
> - You also need a blank line after `TESTS::`.
> - I don't see a reason to catch the `ValueError` being thrown by
`faces`.
>
> Thus, I would make the following changes:
> {{{#!diff
> - def num_faces(self,embedding = None):
> + def num_faces(self, embedding=None):
> """
> - Returns the number of faces of an embedded graph.
> + Return the number of faces of an embedded graph.
>
> EXAMPLES::
>
> sage: T = graphs.TetrahedralGraph()
> sage: T.num_faces()
> 4
>
> TESTS::
> +
> sage: G = graphs.CompleteMultipartiteGraph([3,3])
> sage: G.num_faces()
> Traceback (most recent call last):
> ...
> ValueError: No embedding is provided and the graph is not
planar.
>
> """
>
> - try:
> - return len(self.faces(embedding))
> - except ValueError as e:
> - raise ValueError(e)
> + return len(self.faces(embedding))
> }}}
> Personally I don't like the blank lines around the `"""`, especially on
both sides, but that is much more a stylistic thing.
Thanks for the advice. I pushed the changes.
--
Ticket URL: <http://trac.sagemath.org/ticket/19936#comment:5>
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 https://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
