Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#13899 closed defect (fixed)

Don't use TAB characters for indentation

Reported by: jdemeyer Owned by: jason
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:

Description (last modified by kcrisman)

  • 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 jdemeyer 7 years ago.
13899_TAB_sagelib.patch (148.1 KB) - added by jdemeyer 7 years ago.
13899_TAB_scripts.patch (583 bytes) - added by jdemeyer 7 years ago.
13899_doctest.patch (4.5 KB) - added by jdemeyer 7 years ago.
trac_13899-doctest-rebase.patch (4.2 KB) - added by kcrisman 7 years ago.

Download all attachments as: .zip

Change History (38)

comment:1 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 7 years ago by jdemeyer

  • Description modified (diff)

Changed 7 years ago by jdemeyer

Changed 7 years ago by jdemeyer

Changed 7 years ago by jdemeyer

comment:3 Changed 7 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)

comment:4 Changed 7 years ago by leif

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 7 years ago by leif

  • Reviewers set to Leif Leonhardy
  • Status changed from new to needs_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 7 years ago by leif

  • Status changed from needs_review to positive_review

comment:7 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_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 follow-up: Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

The Sage library patch 13899_doctest.patch needs review.

comment:9 in reply to: ↑ 8 ; follow-ups: Changed 7 years ago by 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. I assume the links for tmp_dir and tmp_filename work fine?

comment:10 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by jdemeyer

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 ; follow-up: Changed 7 years ago by 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.

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 7 years ago by jdemeyer

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 7 years ago by jdemeyer

comment:13 in reply to: ↑ 9 Changed 7 years ago by jdemeyer

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 7 years ago by leif

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 follow-up: Changed 7 years ago by leif

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 7 years ago by kcrisman

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 7 years ago by kcrisman

comment:17 follow-up: Changed 7 years ago by kcrisman

  • 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 7 years ago by kcrisman (previous) (diff)

comment:18 in reply to: ↑ 17 Changed 7 years ago by kcrisman

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 follow-up: Changed 7 years ago by kcrisman

  • Description modified (diff)
  • Reviewers changed from Leif Leonhardy to Leif 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 7 years ago by kcrisman (previous) (diff)

comment:20 in reply to: ↑ 19 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to positive_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 7 years ago by jdemeyer

  • Merged in set to sage-5.6.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:22 follow-up: Changed 7 years ago by vbraun

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 7 years ago by jdemeyer

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 follow-up: Changed 7 years ago by 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.

comment:25 Changed 7 years ago by jason

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

Last edited 7 years ago by jason (previous) (diff)

comment:26 in reply to: ↑ 24 Changed 7 years ago by kcrisman

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 follow-up: Changed 7 years ago by 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.

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

comment:28 in reply to: ↑ 27 Changed 7 years ago by leif

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 7 years ago by jason

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 follow-ups: Changed 7 years ago by 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). 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 7 years ago by jason (previous) (diff)

comment:31 in reply to: ↑ 30 Changed 7 years ago by jdemeyer

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 7 years ago by leif

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 7 years ago by vbraun

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.