Don't use TAB characters for indentation

[jdemeyer]

• 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 (Remove old notebook files #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:

• attachment: 13899_TAB_sage_root.patch to the root repository
• attachment: 13899_TAB_sagelib.patch and attachment: 13899_doctest.patch to the Sage library
• attachment: 13899_TAB_scripts.patch to the scripts repository

Component: misc

Author: Jeroen Demeyer

Reviewer: Leif Leonhardy, Karl-Dieter Crisman

Merged: sage-5.6.beta3

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

[jdemeyer]

Attachment: 13899_TAB_sage_root.patch.gz

[jdemeyer]

Attachment: 13899_TAB_sagelib.patch.gz

[jdemeyer]

Attachment: 13899_TAB_scripts.patch.gz

[jdemeyer]

Author: Jeroen Demeyer

[nexttime]

comment:4

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... ;-) ]

[nexttime]

comment:5

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.

[nexttime]

Reviewer: Leif Leonhardy

[jdemeyer]

comment:7

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.

[jdemeyer]

comment:8

The Sage library patch attachment: 13899_doctest.patch needs review.

[kcrisman]

comment:9

The Sage library patch attachment: 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?

[jdemeyer]

comment:10

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.

[kcrisman]

comment:11

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?).

[jdemeyer]

comment:12

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.

[jdemeyer]

Attachment: 13899_doctest.patch.gz

[jdemeyer]

comment:13

Replying to @kcrisman:

The Sage library patch attachment: 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.

[nexttime]

comment:14

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.)

[nexttime]

comment:15

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.

[kcrisman]

comment:16

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.

[kcrisman]

comment:17

Attachment: trac_13899-doctest-rebase.patch.gz

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).

[kcrisman]

comment:18

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.

[kcrisman]

comment:19

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.

[kcrisman]

Changed reviewer from Leif Leonhardy to Leif Leonhardy, Karl-Dieter Crisman

[jdemeyer]

comment:20

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.

[jdemeyer]

Merged: sage-5.6.beta3

[vbraun]

comment:22

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

[jdemeyer]

comment:23

Replying to @vbraun:

Hopefully beautifying whitespace at least satisfies your OCD.

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

[jasongrout]

comment:24

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.

[jasongrout]

comment:25

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

[kcrisman]

comment:26

Replying to @jasongrout:

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.

[jdemeyer]

comment:27

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?

[nexttime]

comment:28

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.]

[jasongrout]

comment:29

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.

[jasongrout]

comment:30

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.

[jdemeyer]

comment:31

Replying to @jasongrout:

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).

[nexttime]

comment:32

Replying to @jasongrout:

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...

[vbraun]

comment:33

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 ;-)