Opened 6 years ago

Closed 6 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) Commit: 47339dda4c7c145b846a7fe0ade37d7050069097
Dependencies: Stopgaps:

Description (last modified by ncohen)

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 6 years ago by ncohen

  • Authors set to Nathann Cohen
  • Branch set to u/ncohen/15551
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by git

  • Commit set to 51571f7d0bc4078dd08a6481a1e1d7f6430dbd1b

Branch pushed to git repo; I updated commit sha1. New commits:

51571f7trac #15551: Rename Graph.trace_faces to Graph.faces

comment:3 Changed 6 years ago by ncohen

  • Description modified (diff)

comment:4 Changed 6 years ago by tscrim

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 robust doctest:...: message?
  • Could you use Return instead of Returns 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 6 years ago by ncohen

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 6 years ago by git

  • Commit changed from 51571f7d0bc4078dd08a6481a1e1d7f6430dbd1b to 47339dda4c7c145b846a7fe0ade37d7050069097

Branch pushed to git repo; I updated commit sha1. New commits:

47339ddtrac #15551: Reviewer's comments
c3b7aectrac #15551: Rebase on 6.1.beta2

comment:7 Changed 6 years ago by tscrim

  • 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 6 years ago by ncohen

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 6 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.