Opened 11 years ago

Closed 11 years ago

#10781 closed enhancement (fixed)

add Shrikhande graph to the common graphs database

Reported by: kini Owned by: jason, ncohen, rlm
Priority: major Milestone: sage-4.7
Component: graph theory Keywords:
Cc: Merged in: sage-4.7.alpha3
Authors: Keshav Kini Reviewers: Nathann Cohen, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by kini)

Part of the larger project at #9136 .

Apply trac_10781-Shrikhande-graph.patch

Attachments (2)

trac_10781-reviewer.patch​ (2.5 KB) - added by ncohen 11 years ago.
trac_10781-Shrikhande-graph.patch​ (4.7 KB) - added by kini 11 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 11 years ago by kini

  • Authors set to Keshav Kini
  • Status changed from new to needs_review

comment:2 Changed 11 years ago by kini

  • Owner changed from kini to jason, ncohen, rlm

Patch added. I'm assigning this ticket to jason, ncohen, and rlm because they are in charge of the blanket ticket #9136 .

comment:3 Changed 11 years ago by ncohen

  • Milestone set to sage-4.7
  • Reviewers set to Nathann Cohen

Nice addition ! Passes all tests and does its work, except for one small detail : we try to keep the lines below 80 characters in Sage's code. I uploaded a reviewer's patch, and if you agree with it this ticket can be set to "positive review"

Nathann

Changed 11 years ago by ncohen

comment:4 follow-up: Changed 11 years ago by kini

Of course, that is fine with me. I'll keep the 80-character width limit in mind next time.

If you like, I can merge your patch into mine and reupload it. Shall I?

comment:5 in reply to: ↑ 4 Changed 11 years ago by ncohen

If you like, I can merge your patch into mine and reupload it. Shall I?

Oh. I did it myself just to avoid you the trouble of doing this. Anyway you're the only Author of this patch, it's just to make the review quicker :-)

So it's up to you : just don't forget to set the ticket to "positive review" so that the release manager will eventually merge the two tickets !

Nathann

comment:6 follow-up: Changed 11 years ago by ncohen

Ping ? :-)

Nathann

comment:7 in reply to: ↑ 6 Changed 11 years ago by kini

  • Description modified (diff)
  • Status changed from needs_review to positive_review

Replying to ncohen:

Ping ? :-)

Nathann

Hey, sorry for the delay, I was traveling (on my way, indirectly, to Sage Days 29 !). I'm setting this ticket to "positive review" as you asked, but if you change your mind feel free to change it back :)

comment:8 Changed 11 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Since the hyperlink to the MathWorld and Wikipedia articles are not meant to be refered to from a different place, you should make links with two trailing underscores. See ​http://docutils.sourceforge.net/docs/ref/rst/restructuredtext.html#embedded-uris

Changed 11 years ago by kini

comment:9 follow-up: Changed 11 years ago by kini

  • Status changed from needs_work to needs_review

Hi Jeroen, thanks for your advice as usual! After looking through the link you provided, and also the file I am modifying ( sage/graphs/graph_generators.py ), it seems that all the other links to Wikipedia or MathWorld? are using single underscores. The only difference seemed to be that in my links, the text of the link (i.e. the name given to the reference) was attached to the context of the sentence it was in, rather than being a standalone phrase. I attempted to fix that, instead of doubling the underscores. Is that acceptable?

comment:10 in reply to: ↑ 9 Changed 11 years ago by jdemeyer

Replying to kini:

Hi Jeroen, thanks for your advice as usual! After looking through the link you provided, and also the file I am modifying ( sage/graphs/graph_generators.py ), it seems that all the other links to Wikipedia or MathWorld? are using single underscores. The only difference seemed to be that in my links, the text of the link (i.e. the name given to the reference) was attached to the context of the sentence it was in, rather than being a standalone phrase. I attempted to fix that, instead of doubling the underscores. Is that acceptable?

I guess this is acceptable, yes. I still have to build the documentation and see what it gives, but I guess it's okay.

comment:11 Changed 11 years ago by jdemeyer

  • Reviewers changed from Nathann Cohen to Nathann Cohen, Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:12 Changed 11 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.