Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#13899 closed defect (fixed)

Don't use TAB characters for indentation

Reported by: Jeroen Demeyer Owned by: Jason Grout
Priority: trivial Milestone: sage-5.6
Component: misc Keywords:
Cc: Merged in: sage-5.6.beta3
Authors: Jeroen Demeyer Reviewers: Leif Leonhardy, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Karl-Dieter Crisman)

  • TABs in Makefiles are needed, so keep them.
  • Some TABs not used for indentation are also kept.
  • TABs in the obsolete directory devel/sage/sage/server/notebook (#11409) are not fixed.
  • The directory sage/graphs/planarity_c uses a very inconsistent indentation scheme (mixing spaces and TABs and having varying amounts of indentation) is hopeless to easily fix automatically.
  • Also, make indentation consistent and/or remove trailing spaces in some places.

The release manager script has been updated to check all new patches for added TAB indentation.

Apply:

Attachments (5)

13899_TAB_sage_root.patch (1.8 KB) - added by Jeroen Demeyer 10 years ago.
13899_TAB_sagelib.patch (148.1 KB) - added by Jeroen Demeyer 10 years ago.
13899_TAB_scripts.patch (583 bytes) - added by Jeroen Demeyer 10 years ago.
13899_doctest.patch (4.5 KB) - added by Jeroen Demeyer 10 years ago.
trac_13899-doctest-rebase.patch (4.2 KB) - added by Karl-Dieter Crisman 10 years ago.

Download all attachments as: .zip

Change History (38)

comment:1 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)

comment:2 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)

Changed 10 years ago by Jeroen Demeyer

Attachment: 13899_TAB_sage_root.patch added

Changed 10 years ago by Jeroen Demeyer

Attachment: 13899_TAB_sagelib.patch added

Changed 10 years ago by Jeroen Demeyer

Attachment: 13899_TAB_scripts.patch added

comment:3 Changed 10 years ago by Jeroen Demeyer

Authors: Jeroen Demeyer
Description: modified (diff)

comment:4 Changed 10 years ago by Leif Leonhardy

Patches look sane (yes, I've read the whole Sage library patch!), haven't tried to apply them yet though.

Other than removing trailing whitespace (and removing useless backslashes at the end of a few lines), I don't see much value in touching the C/C++ and HTML files. In particular, removing tabs that introduce end-of-line comments (in the C/C++ files) is rather counter-productive. [Instead, it would have been worth to translate the French ones, and especially French error messages... ;-) ]

comment:5 Changed 10 years ago by Leif Leonhardy

Reviewers: Leif Leonhardy
Status: newneeds_review

Not sure whether you planned to add further patches here; just noticed it was still "new".

Patches apply to Sage 5.6.beta1, doc builds... (The Sage library patch btw. triggers the rebuild of half of the Sage library.)

Haven't reviewed the merger script though.

comment:6 Changed 10 years ago by Leif Leonhardy

Status: needs_reviewpositive_review

comment:7 Changed 10 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work

There are actually doctest failures in conventions.rst, I guess because of a bug in the doctest script which causes the tests not to be run.

comment:8 Changed 10 years ago by Jeroen Demeyer

Status: needs_workneeds_review

The Sage library patch 13899_doctest.patch needs review.

comment:9 in reply to:  8 ; Changed 10 years ago by Karl-Dieter Crisman

The Sage library patch 13899_doctest.patch needs review.

This patch needs some work. At least you should remove the remaining reference(s?) to cdd_convert. Also, I'm a little surprised that you indented the last two examples by one more - shouldn't they be exactly four spaces from the start of the colon (I mean that column)? I'm not sure why some of the doc has three spaces to the colon and then four more, but I've seen it before. I assume the links for tmp_dir and tmp_filename work fine?

comment:10 in reply to:  9 ; Changed 10 years ago by Jeroen Demeyer

Replying to kcrisman:

I assume the links for tmp_dir and tmp_filename work fine?

No they don't, but I think links between two different manuals (in this case, from the developer manual to the reference manual) are not supported.

comment:11 in reply to:  10 ; Changed 10 years ago by Karl-Dieter Crisman

I assume the links for tmp_dir and tmp_filename work fine?

No they don't, but I think links between two different manuals (in this case, from the developer manual to the reference manual) are not supported.

Good point. But in that case should they just be

`tmp_dir`

instead of

:func:`tmp_dir`

or does it just ignore this and not make a link? I'm surprised it doesn't raise an error in building the doc (or does it?).

comment:12 in reply to:  11 Changed 10 years ago by Jeroen Demeyer

Replying to kcrisman:

or does it just ignore this and not make a link?

That's exactly what happens. If we ever allow inter-document links (does #6495 solve this?) the links should magically start working.

Changed 10 years ago by Jeroen Demeyer

Attachment: 13899_doctest.patch added

comment:13 in reply to:  9 Changed 10 years ago by Jeroen Demeyer

Replying to kcrisman:

The Sage library patch 13899_doctest.patch needs review.

This patch needs some work. At least you should remove the remaining reference(s?) to cdd_convert. Also, I'm a little surprised that you indented the last two examples by one more - shouldn't they be exactly four spaces from the start of the colon (I mean that column)? I'm not sure why some of the doc has three spaces to the colon and then four more, but I've seen it before.

Okay, addressed your comments. New patch ready for review.

comment:14 Changed 10 years ago by Leif Leonhardy

The example now using tmp_filename() (instead of SAGE_TMP) could probably be improved, as the temporary file's name immediately gets discarded. (I think one would typically reuse it, e.g. to load a saved file later in an example / doctest.)

comment:15 Changed 10 years ago by Leif Leonhardy

Otherwise Developer's Guide builds and all of its doctests pass now (still with beta1 though), so positive review again if Karl-Dieter's ok with it, too.

comment:16 in reply to:  15 Changed 10 years ago by Karl-Dieter Crisman

Otherwise Developer's Guide builds and all of its doctests pass now (still with beta1 though), so positive review again if Karl-Dieter's ok with it, too.

Believe it or not, this does not apply to beta2. The first hunk is now

        def __init__(self, S, m):
            """
            See ``HillCryptosystem`` for full documentation.

            EXAMPLES::
            ...
            """

which is close enough to this patch not to warrant extra intrusion.

Changed 10 years ago by Karl-Dieter Crisman

comment:17 Changed 10 years ago by Karl-Dieter Crisman

Description: modified (diff)

Patchbot, apply 13899_TAB_sage_root.patch to root, 13899_TAB_sagelib.patch and trac_13899-doctest-rebase.patch to the Sage library, and 13899_TAB_scripts.patch to the scripts repository.

I have NOT looked at the other patches, only the doctest fix. I do give positive review to the doctest fix (with my trivial rebase).

Last edited 10 years ago by Karl-Dieter Crisman (previous) (diff)

comment:18 in reply to:  17 Changed 10 years ago by Karl-Dieter Crisman

Replying to kcrisman:

Patchbot, apply 13899_TAB_sage_root.patch to root, 13899_TAB_sagelib.patch and trac_13899-doctest-rebase.patch to the Sage library, and 13899_TAB_scripts.patch to the scripts repository.

I have NOT looked at the other patches, only the doctest fix. I do give positive review to the doctest fix (with my trivial rebase).

Scratch that - I see what happened, Jeroen introduced the problem in his previous patch and then fixed it. Now I have to wait for all those Cython files to rebuild to check how nice the developer guide looks... one moment.

comment:19 Changed 10 years ago by Karl-Dieter Crisman

Description: modified (diff)
Reviewers: Leif LeonhardyLeif Leonhardy, Karl-Dieter Crisman

Patchbot, apply 13899_TAB_sage_root.patch to the root repository, 13899_TAB_sagelib.patch and 13899_doctest.patch to the Sage library, and 13899_TAB_scripts.patch to the scripts repository.

Okay, Jeroen's doctest patch is fine. Sorry for the confusion.

Last edited 10 years ago by Karl-Dieter Crisman (previous) (diff)

comment:20 in reply to:  19 Changed 10 years ago by Jeroen Demeyer

Status: needs_reviewpositive_review

Replying to kcrisman:

Okay, Jeroen's doctest patch is fine. Sorry for the confusion.

Since leif already reviewed the other patches, I guess this means positive review then.

comment:21 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.6.beta3
Resolution: fixed
Status: positive_reviewclosed

comment:22 Changed 10 years ago by Volker Braun

This breaks the collection of 25 patches for IPython 0.13. Hopefully beautifying whitespace at least satisfies your OCD.

comment:23 in reply to:  22 Changed 10 years ago by Jeroen Demeyer

Replying to vbraun:

Hopefully beautifying whitespace at least satisfies your OCD.

And Guido van Rossums: http://www.python.org/dev/peps/pep-0008/#indentation

comment:24 Changed 10 years ago by Jason Grout

Whew; -1 on massive whitespace changes like this unless they are actual errors because it breaks patches, which can be a lot of work to rebase. I thought we had a whitespace beautifying discussion on sage-devel a while ago and we decided on the strategy of correcting whitespace as it was naturally encountered in code changes. At least, that's what I thought.

comment:25 Changed 10 years ago by Jason Grout

I guess it's this that I have the most objection too: "Also, make indentation consistent and/or remove trailing spaces in some places. "

Version 0, edited 10 years ago by Jason Grout (next)

comment:26 in reply to:  24 Changed 10 years ago by Karl-Dieter Crisman

Replying to jason:

Whew; -1 on massive whitespace changes like this unless they are actual errors because it breaks patches, which can be a lot of work to rebase. I thought we had a whitespace beautifying discussion on sage-devel a while ago and we decided on the strategy of correcting whitespace as it was naturally encountered in code changes. At least, that's what I thought.

Sorry, guys, I guess I should have caught this because I recall that discussion too. (And implemented it or something similar in some cases - see my comment at #13255.) I was just trying to help out with the doctest patch, but somehow didn't really think of the rest as it had been "taken care of", but that's no excuse.

comment:27 Changed 10 years ago by Jeroen Demeyer

I apologize for the trouble. It's true that I'm usually against there changes, but I somehow thought (apparently a mistake) that this was mostly code which isn't often changed.

Jason: which is the patch you're having trouble with rebasing?

comment:28 in reply to:  27 Changed 10 years ago by Leif Leonhardy

Replying to jdemeyer:

I apologize for the trouble. It's true that I'm usually against there changes, but I somehow thought (apparently a mistake) that this was mostly code which isn't often changed.

TBH, I was pretty aware of the discussions in the past when reviewing this, but regarding the files touched, didn't expect major breakage -- just like Jereon. So to avoid reopening the lengthy debates of the past, I tried to quickly review it... ;-)

[And as mentioned, changing C/C++ and HTML files doesn't make that much sense to me, although those changes probably don't cause the trouble you may have now.]

comment:29 Changed 10 years ago by Jason Grout

The big set of patches that caught my attention and made me aware of this ticket was the IPython ticket, #12719, that Volker already rebased (see the comment above by him, which I think shows his frustration with having to rebase things). I wouldn't be surprised if there were lots of other patches affected by some of the whitespace changes.

comment:30 Changed 10 years ago by Jason Grout

For the record, I would be +1 on reverting the parts of this patch that are touching lots of files for beauty's sake only (like removing trailing whitespace). Large rebasing of tickets like #12719 could easily introduce subtle code errors, and not forcing those changes should take priority over end-of-line spacing.

Last edited 10 years ago by Jason Grout (previous) (diff)

comment:31 in reply to:  30 Changed 10 years ago by Jeroen Demeyer

Replying to jason:

For the record, I would be +1 on reverting the parts of this patch that are touching lots of files for beauty's sake only (like removing trailing whitespace)

Apart from the changes to conventions.rst, this patch is only about touching files for beauty's sake. Given that #12719 is already rebased to this ticket, unmerging would be unwise. It seems that #12719 (which is a huge patch) conflicts with this in only one hunk, for a file which is removed in its totality.

So it's not a total disaster and it's not contradictory to leif's and my idea that these patches only touch rarely-edited files (except conventions.rst but that file really needed to be fixed).

comment:32 in reply to:  30 Changed 10 years ago by Leif Leonhardy

Replying to jason:

Large rebasing of tickets like #12719 could easily introduce subtle code errors, and not forcing those changes should take priority over end-of-line spacing.

Just checked, and as far as I can see, the only offending patch here was removing an empty line from a file that #12719 removes in its entirety. So I wouldn't say we broke much here, at least until now...

In general, patches IMHO shouldn't touch parts they don't have to (i.e., only doing some reformatting there), which we agreed on IIRC. But in this case, the purpose of the ticket is to remove tabs (and "normalize" code -- once and for all -- hopefully), so it does not touch code it doesn't have to...

comment:33 Changed 10 years ago by Volker Braun

Automatically enforcing code style is nice but should only be done though local commit hooks. Having a check in Jeroen's release script is BS, that just adds a completely avoidable multi-week ping-pong game when trying to merge a ticket.

In any case this ticket is done so lets leave it at that. But please post to sage-devel next time so we can talk you out of it ;-)

Note: See TracTickets for help on using tickets.