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.

Sign up for GitHub

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

Jump to bottom

pep8 in digraph_generators.py (part 2) #27135

Closed
dcoudert opened this issue Jan 26, 2019 · 22 comments
Closed

pep8 in digraph_generators.py (part 2) #27135

dcoudert opened this issue Jan 26, 2019 · 22 comments
Labels
c: graph theory p: minor / 4 t: enhancement
Milestone
sage-8.7

Comments

@dcoudert
Copy link
Contributor

Clean

  • Complete
  • Circuit
  • Circulant

In both Circuit and Circulant, we also avoid creating the list of edges before adding it to the digraph (small speed up)

Component: graph theory

Author: David Coudert

Branch/Commit: 5960ed4

Reviewer: Bryan Gin-ge Chen

Issue created by migration from https://trac.sagemath.org/ticket/27135
@dcoudert dcoudert added this to the sage-8.7 milestone Jan 26, 2019
@dcoudert
Copy link
Contributor Author

Commit: aaf7d59

@dcoudert
Copy link
Contributor Author

Branch: u/dcoudert/27135_digraph_generators_2

@dcoudert
Copy link
Contributor Author

New commits:

aaf7d59trac #27135: clean digraph_generators part 2

@dcoudert
Copy link
Contributor Author

Author: David Coudert

@bryangingechen
Copy link
Mannequin

bryangingechen mannequin commented Jan 29, 2019

Reviewer: Bryan Gin-ge Chen

@bryangingechen
Copy link
Mannequin

bryangingechen mannequin commented Jan 29, 2019

comment:4

In the documentation of Circuit, there are missing periods at the ends of sentences.

In Circulant, the 3 occurrences of relative integers should just be integers (I guess this is a French-to-English thing).

Feel free to set to positive review after fixing.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 29, 2019

Changed commit from aaf7d59 to 3748045

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 29, 2019

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

2ac1095trac #27135: Merged with 8.7.beta1
3748045trac #27135: remove occurence of relative

@dcoudert
Copy link
Contributor Author

comment:6

Thank you for the review.

@dcoudert

This comment has been minimized.

Sign in to view
@bryangingechen
Copy link
Mannequin

bryangingechen mannequin commented Jan 29, 2019

comment:7

Oh sorry, I edited my review comment last night and I guess you didn't see it. There are still missing periods at the ends of the sentences in the docstring of Circuit. (Again, feel free to set to positive review afterwards).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 30, 2019

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

39ce464trac #27135: add missing periods in Circuit

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 30, 2019

Changed commit from 3748045 to 39ce464

@dcoudert
Copy link
Contributor Author

comment:10

I added the missing periods. Thank you for the review.

@vbraun
Copy link
Member

vbraun commented Feb 4, 2019

comment:11

Merge conflict

@dcoudert
Copy link
Contributor Author

dcoudert commented Feb 5, 2019

comment:12

OK. I guess I have to wait until next beta to find the issue.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 9, 2019

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

5960ed4trac #27135: fix merge conflict with 8.7.beta3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 9, 2019

Changed commit from 39ce464 to 5960ed4

@dcoudert
Copy link
Contributor Author

dcoudert commented Feb 9, 2019

comment:14

Rebased on 8.7.beta3 and fix merge conflict.

@dcoudert
Copy link
Contributor Author

dcoudert commented Mar 9, 2019

comment:15

Can I set this ticket to positive review ?

@bryangingechen
Copy link
Mannequin

bryangingechen mannequin commented Mar 9, 2019

comment:16

Sorry that I missed this. It looks good to me!

@vbraun
Copy link
Member

vbraun commented Mar 11, 2019

Changed branch from u/dcoudert/27135_digraph_generators_2 to 5960ed4

@vbraun vbraun closed this as completed in 73d5bfd Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: graph theory p: minor / 4 t: enhancement
Projects
None yet
Milestone
sage-8.7
Development

No branches or pull requests
2 participants
@vbraun @dcoudert