Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

little cleanup of hexad.py #20280

Closed
fchapoton opened this issue Mar 24, 2016 · 17 comments
Closed

little cleanup of hexad.py #20280

fchapoton opened this issue Mar 24, 2016 · 17 comments

Comments

@fchapoton
Copy link
Contributor

in particular to get rid of an old-style class

CC: @jdemeyer @tscrim @jm58660

Component: game theory

Author: Frédéric Chapoton

Branch/Commit: b310177

Reviewer: Jori Mäntysalo, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/20280

@fchapoton fchapoton added this to the sage-7.2 milestone Mar 24, 2016
@fchapoton
Copy link
Contributor Author

New commits:

b41e411little cleanup of hexad.py

@fchapoton
Copy link
Contributor Author

Branch: public/20280

@fchapoton
Copy link
Contributor Author

Commit: b41e411

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 24, 2016

Changed commit from b41e411 to e50d9c6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 24, 2016

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

e50d9c6trac #20280 correct links to methods

@tscrim
Copy link
Collaborator

tscrim commented Apr 3, 2016

comment:5

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 6, 2016

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 6, 2016

Changed commit from e50d9c6 to 79cf34f

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Apr 7, 2016

comment:7

"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?

@fchapoton
Copy link
Contributor Author

comment:8

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.

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Apr 8, 2016

comment:9

Replying to @fchapoton:

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?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 8, 2016

Changed commit from 79cf34f to b310177

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 8, 2016

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.

@tscrim
Copy link
Collaborator

tscrim commented Apr 8, 2016

comment:11

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.

@tscrim
Copy link
Collaborator

tscrim commented Apr 8, 2016

Reviewer: Jori Mäntysalo, Travis Scrimshaw

@fchapoton
Copy link
Contributor Author

comment:12

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

@vbraun
Copy link
Member

vbraun commented Apr 10, 2016

Changed branch from public/20280 to b310177

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants