Re: [sage-devel] Code style guidelines
On 2011-10-24 16:54, Keshav Kini wrote: > If we take some stance on it, this will be reduced. [citation needed] It's obvious that it's better not to have trailing whitespace in files. I'm not against adding a section in the developer's manual for this but I doubt it will make any difference in practice. If people use editors which are prone to trailing whitespace, that is not going to change. -- To post to this group, send an email to sage-devel@googlegroups.com To unsubscribe from this group, send an email to sage-devel+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/sage-devel URL: http://www.sagemath.org
Re: [sage-devel] Code style guidelines
The main point is that it would add legitimacy to people insisting that patches not contain trailing whitespace before giving them positive review on trac :) -Keshav Join us in #sagemath on irc.freenode.net ! -- To post to this group, send an email to sage-devel@googlegroups.com To unsubscribe from this group, send an email to sage-devel+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/sage-devel URL: http://www.sagemath.org
Re: [sage-devel] Code style guidelines
By the way, I think trailing whitespace is often intentional. Some people consider this a good thing: Some text$ $ Some more text$ Some people also consider this a good thing: -$ TITLE$ -$ Hell, I used to think these were good things until someone knocked some sense into my head :P And this kind of trailing whitespace (i.e. whitespace for making $ line up with things) accounts for a good portion of the trailing whitespace I see in Sage's codebase. Of 54837 instances of trailing whitespace in files in $SAGE_ROOT/devel/sage/sage , 42576 occurred on otherwise empty lines, which I assume mostly falls under the first example given above. -Keshav Join us in #sagemath on irc.freenode.net ! -- To post to this group, send an email to sage-devel@googlegroups.com To unsubscribe from this group, send an email to sage-devel+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/sage-devel URL: http://www.sagemath.org
Re: [sage-devel] Code style guidelines
On 2011-10-24 17:19, Keshav Kini wrote: > By the way, I think trailing whitespace is often intentional. Some > people consider this a good thing: > > Some text$ > $ > Some more text$ Personally, I do NOT consider this a problem. I would like to remove whitespace after non-empty lines, but I don't really care much about whitespace-only lines. -- To post to this group, send an email to sage-devel@googlegroups.com To unsubscribe from this group, send an email to sage-devel+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/sage-devel URL: http://www.sagemath.org
Re: [sage-devel] Code style guidelines
On Mon, 24 Oct 2011 at 07:54AM -0700, Keshav Kini wrote: > Shouldn't we have mention trailing whitespace somewhere in the developer's > guide - namely telling people to omit it? Having such a policy makes for > cleaner revision control, as most agree (for example, at some point git by > default didn't let you commit changes which include adding trailing > whitespace to files). There are way too many patches on trac with chunks > that just remove trailing whitespace introduced in other patches, IMHO. If > we take some stance on it (much like our official endorsement of PEP 8), > this will be reduced. If people agree I'll make a ticket to add a couple > lines to the developer's guide. Based on the no-tabs-in-Sage-code experience, the only way to have any kind of actual effect is to add a tool to the merge or doctest scripts so that new trailing whitespace causes an error. (Working on Sage has taught me that only automated tools work; asking people to remember something or behave a certain way always fails. :) It seems like no one wants to have an epic break-all-patches-in-trac patch that removes all the trailing whitespace in the Sage library -- and, some people like some trailing whitespace, so perhaps we should also add just a warning to the sage-merge script (or whatever release managers use to add patches)? Dan -- --- Dan Drake - http://mathsci.kaist.ac.kr/~drake --- signature.asc Description: Digital signature
Re: [sage-devel] Code style guidelines
On 2011-10-25 02:33, Dan Drake wrote: > It seems like no one wants to have an epic break-all-patches-in-trac > patch that removes all the trailing whitespace in the Sage library -- > and, some people like some trailing whitespace, so perhaps we should > also add just a warning to the sage-merge script (or whatever release > managers use to add patches)? The following would be easy for me to do: remove all *newly added* trailing whitespace from patches (except on lines consisting only of spaces). Basically, s/^\(+.*[^ ]\) *$/\1/ in the patches. -- To post to this group, send an email to sage-devel@googlegroups.com To unsubscribe from this group, send an email to sage-devel+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/sage-devel URL: http://www.sagemath.org
Re: [sage-devel] Code style guidelines
On Tue, 25 Oct 2011 at 08:37AM +0200, Jeroen Demeyer wrote: > The following would be easy for me to do: remove all *newly added* > trailing whitespace from patches (except on lines consisting only of > spaces). Basically, s/^\(+.*[^ ]\) *$/\1/ in the patches. I think that would be a good idea, although if you're just running that through sed, the exact patches applied would be different from the ones on the trac server, which would cause some confusion. Dan -- --- Dan Drake - http://mathsci.kaist.ac.kr/~drake --- signature.asc Description: Digital signature
Re: [sage-devel] Code style guidelines
On 2011-10-25 09:51, Dan Drake wrote: > I think that would be a good idea, although if you're just running that > through sed, the exact patches applied would be different from the ones > on the trac server, which would cause some confusion. Very true, but I think that is only a minor annoyance. The point is that I don't like warnings. There used to be time when not having a ticket number in the commit message created a warning in my merger script. This meant I had to complain on a lot of tickets about the commit message. Eventually, people were asking, "why doesn't your script automatically add the ticket number?". Since then, that's how things are done in the merger script. No warnings, just automatically (and silently) make the change. Jeroen. -- To post to this group, send an email to sage-devel@googlegroups.com To unsubscribe from this group, send an email to sage-devel+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/sage-devel URL: http://www.sagemath.org
Re: [sage-devel] Code style guidelines
On 2011-10-25 08:37, Jeroen Demeyer wrote: > The following would be easy for me to do: remove all *newly added* > trailing whitespace from patches (except on lines consisting only of > spaces). Basically, s/^\(+.*[^ ]\) *$/\1/ in the patches. I just tried to do this for sage-4.8.alpha1 and the problem is reviewer patches. If a ticket has a "main" patch adding new trailing whitespace and a "reviewer" ticket changing those lines, then removing new trailing whitespace from patches creates conflicts. -- To post to this group, send an email to sage-devel@googlegroups.com To unsubscribe from this group, send an email to sage-devel+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/sage-devel URL: http://www.sagemath.org
