Opened 7 years ago

Closed 7 years ago

#20280 closed enhancement (fixed)

little cleanup of hexad.py

Reported by: Frédéric Chapoton Owned by:
Priority: minor Milestone: sage-7.2
Component: game theory Keywords:
Cc: Jeroen Demeyer, Travis Scrimshaw, Jori Mäntysalo Merged in:
Authors: Frédéric Chapoton Reviewers: Jori Mäntysalo, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: b310177 (Commits, GitHub, GitLab) Commit: b31017743014cb21bfb0594af6fb9383e59680b3
Dependencies: Stopgaps:

Status badges

Description

in particular to get rid of an old-style class

Change History (13)

comment:1 Changed 7 years ago by Frédéric Chapoton

Branch: public/20280
Commit: b41e41199c071fc931b6fc56aa58b55044f692cc
Status: newneeds_review

New commits:

b41e411little cleanup of hexad.py

comment:2 Changed 7 years ago by Frédéric Chapoton

Cc: Jeroen Demeyer added

comment:3 Changed 7 years ago by git

Commit: b41e41199c071fc931b6fc56aa58b55044f692cce50d9c63315f3641ba3f184b8b82679fa11a9b56

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

e50d9c6trac #20280 correct links to methods

comment:4 Changed 7 years ago by Frédéric Chapoton

Cc: Travis Scrimshaw Jori Mäntysalo added

comment:5 Changed 7 years ago by Travis Scrimshaw

A few comments:

  • Can you put a line break in the See the docstrings... sentence?
  • Why does Minimog need to inherit from SageObject? Can't it just inherit from object to be a new-style (Python) class?
  • Should we (I am willing to do these too) also do the following:
    • Make the error message start with a lowercase letter and not end in a period.
    • Put some of the module level doc into latex/code formatting.
    • Cleanup the code of print_kitten.
Last edited 7 years ago by Travis Scrimshaw (previous) (diff)

comment:6 Changed 7 years ago by git

Commit: e50d9c63315f3641ba3f184b8b82679fa11a9b5679cf34fd012a55da6b66d13907f8721d4b0e7f86

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

dfe00e8Merge branch 'public/20280' into 7.2.b2
79cf34fsome of reviewer's comments

comment:7 Changed 7 years ago by Jori Mäntysalo

"This is needed in the find_hexad function below."

Is there direct use for this function? If not, why it is not internal function or have a name that starts with underscore?

comment:8 Changed 7 years ago by Frédéric Chapoton

ok, guys, this was just a small cleanup ticket. If you want to make this file perfect, this will take a lot of work. I do not think this is worth spending our time here.

comment:9 in reply to:  8 Changed 7 years ago by Jori Mäntysalo

Replying to chapoton:

ok, guys, this was just a small cleanup ticket. If you want to make this file perfect, this will take a lot of work. I do not think this is worth spending our time here.

Sorry about that. Otherwise changes seems to be good, but I don't know about those imports. Actually I know quite small part of Sage.

The code compiled and passed tests, thought. Should I lower my limit of giving positive review?

comment:10 Changed 7 years ago by git

Commit: 79cf34fd012a55da6b66d13907f8721d4b0e7f86b31017743014cb21bfb0594af6fb9383e59680b3

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

131c2a4Merge branch 'public/20280' of trac.sagemath.org:sage into public/20280
b310177A little more cleanup.

comment:11 Changed 7 years ago by Travis Scrimshaw

Reviewers: Jori Mäntysalo, Travis Scrimshaw

I've done a little bit more cleanup of the file.

In regards to comment:7, this is not imported to the global namespace and having it appear in the doc is not necessarily a bad thing.

If you agree with my changes, then you can set a positive review.

comment:12 Changed 7 years ago by Frédéric Chapoton

Status: needs_reviewpositive_review

ok,looks good to me. Let us go on to something else.

comment:13 Changed 7 years ago by Volker Braun

Branch: public/20280b31017743014cb21bfb0594af6fb9383e59680b3
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.