Opened 7 years ago
Closed 7 years ago
#15551 closed enhancement (fixed)
Rename Graph.trace_faces to Graph.faces
Reported by: | ncohen | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.1 |
Component: | graph theory | Keywords: | |
Cc: | fidelbarrera | Merged in: | |
Authors: | Nathann Cohen | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | u/ncohen/15551 (Commits, GitHub, GitLab) | Commit: | 47339dda4c7c145b846a7fe0ade37d7050069097 |
Dependencies: | Stopgaps: |
Description (last modified by )
Somebody asked on AskSage
how to compute the faces of a graph. And well, one wouldn't think of looking for trace_faces
, to let's rename it :-P
This ticket also renames (with a deprecated alias) the method check_embedding_validity
and check_pos_validity
to _check_embedding_validity
and _check_pos_validity
. These things are internal things and should stay internal.
Nathann
Change History (9)
comment:1 Changed 7 years ago by
- Branch set to u/ncohen/15551
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
- Commit set to 51571f7d0bc4078dd08a6481a1e1d7f6430dbd1b
comment:3 Changed 7 years ago by
- Description modified (diff)
comment:4 Changed 7 years ago by
A few minor things:
- The alignment of the
``embedding``
input bullet-point is off by a space (lines 3729-3734). - Instead of
doctest:1: message
could you change it to the more robustdoctest:...: message
? - Could you use
Return
instead ofReturns
on line 3721? - I believe the error messages should be formatted as
ValueError("lower case with no period")
.
Otherwise looks good. Thanks.
comment:5 Changed 7 years ago by
Helloooooooooooo ! I fixed all of those except the last one. I don't think that it is soooo dangerous, plus I often write (informative) exceptions messages with several sentences (in which case upper case and periods are needed), and I think it helps ^^;
Nathann
comment:6 Changed 7 years ago by
- Commit changed from 51571f7d0bc4078dd08a6481a1e1d7f6430dbd1b to 47339dda4c7c145b846a7fe0ade37d7050069097
comment:7 Changed 7 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
I thought that there was some PEP on this...but I can't find it, so it's a consistency with python/personal preference. *shrugs*
Positive review. Thanks Nathann.
comment:8 Changed 7 years ago by
There should be a PEP against PEP. Thaaaaanks for the review :-)
Gosh, these days Sage is the only place where stuff is actually happening, and where people actually work toward something O_o
Nathann
comment:9 Changed 7 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
trac #15551: Rename Graph.trace_faces to Graph.faces