Removing tabs in .rst, .tex and .pxi files
Closed this issue · 26 comments
The new doctesting framework (#12415) has revealed tab characters that weren't caught by the old. This ticket changes them to spaces.
Apply attachment: 13146_vs_51b5.patch and attachment: 13146_missed.patch
Component: doctest coverage
Author: David Roe
Reviewer: Keshav Kini, André Apitzsch, Jeroen Demeyer
Merged: sage-5.2.rc0
Issue created by migration from https://trac.sagemath.org/ticket/13146
Needs a real commit message, otherwise looks good :)
Attachment: 13146_vs_51b5.patch.gz
Changes to apply against 5.1.beta5
Description changed:
---
+++
@@ -1 +1,3 @@
The new doctesting framework (#12415) has revealed tab characters that weren't caught by the old. This ticket changes them to spaces.
+
+Apply only [attachment: 1316_vs_51b5.patch.](https://github.com/sagemath/sage/files/ticket13146/1316_vs_51b5.patch..gz)There is now a real commit message.
patchbot: apply 13146_vs_51b5.patch
Description changed:
---
+++
@@ -1,3 +1,3 @@
The new doctesting framework (#12415) has revealed tab characters that weren't caught by the old. This ticket changes them to spaces.
-Apply only [attachment: 1316_vs_51b5.patch.](https://github.com/sagemath/sage/files/ticket13146/1316_vs_51b5.patch..gz)
+Apply only [attachment: 13146_vs_51b5.patch.](https://github.com/sagemath/sage/files/ticket13146/13146_vs_51b5.patch..gz)Cool, passes tests.
Reviewer: Keshav Kini
I missed a few that were added between 5.0.beta12 and 5.1.beta5. I wanted to just add them here since it's a small change (though if you like I can make another ticket).
Fixed. :)
Changed reviewer from Keshav Kini to Keshav Kini, André Apitzsch
You probably added by accident
Index: sage/schemes/elliptic_curves/constructor.py
===================================================================
--- a/sage/schemes/elliptic_curves/constructor.py
+++ b/sage/schemes/elliptic_curves/constructor.py
@@ -1,6 +1,8 @@
"""
Elliptic curve constructor
+sage: EllipticCurve([0,0])
+
AUTHORS:
- William Stein (2005): Initial versionPlease remove this.
Description changed:
---
+++
@@ -1,3 +1,3 @@
The new doctesting framework (#12415) has revealed tab characters that weren't caught by the old. This ticket changes them to spaces.
-Apply only [attachment: 13146_vs_51b5.patch.](https://github.com/sagemath/sage/files/ticket13146/13146_vs_51b5.patch..gz)
+Apply [attachment: 13146_vs_51b5.patch](https://github.com/sagemath/sage-prod/files/10655827/13146_vs_51b5.patch.gz) and [attachment: 13146_missed.patch](https://github.com/sagemath/sage-prod/files/10655828/13146_missed.patch.gz)Sorry about that. qrefresh -e committed some unintended changes.
With #12415, I still get:
sage -t devel/sage/doc/ru/tutorial/appendix.rst # Tab character found
sage -t devel/sage/doc/de/tutorial/interactive_shell.rst # Tab character found
sage -t devel/sage/doc/de/tutorial/tour_functions.rst # Tab character found
sage -t devel/sage/doc/ru/tutorial/tour_functions.rst # Tab character found
Can someone explain the point of this ticket? The reason that we avoid tab characters in .py files is that Sphinx can't handle them, so Sphinx doesn't work for processing docstrings for introspection in the notebook. Tabs in these other files don't cause problems, so why worry about removing them? Why bother testing for them in #12415?
Another reason to avoid tab characters in .py files is because they can be confusing to anyone trying to read them with an editor whose tab stops are set to something other than 8 characters, if tabs and spaces are mixed (because indentation levels may appear to differ from what the python interpreter actually judges them to be). The python interpreter has a command line switch -t which issues warnings when such a condition exists, for example.
reStructuredText, like Python code, is indentation-sensitive, and so tabs should be avoided in reStructuredText for the same reason they should be avoided in Python code. The same is true of .pxi files.
Dunno why we're testing for tabs in .tex files, though.
The location of the test changed, and it was easier to just test for tabs in all of the files that we doctest rather than special case the test based on file type. I can change it to allow tabs if people like and make this ticket invalid.
Personally I'm all for getting rid of tabs everywhere. Either tabs are used semantically or not, and clearly in Sage they're not.
Replying to @roed314:
The location of the test changed, and it was easier to just test for tabs in all of the files that we doctest rather than special case the test based on file type.
That makes sense. I certainly don't mind getting rid of tabs, but a concerted effort to remove all of them needs a good reason, and this is as good a reason as any.
Attachment: 13146_missed.patch.gz
Missed a few
Alright, I fixed the tabs in the Russian and German documentation.
Changed reviewer from Keshav Kini, André Apitzsch to Keshav Kini, André Apitzsch, Jeroen Demeyer
Merged: sage-5.2.rc0
By the way, here's a good reason to disallow tabs in .rst files. When scanning a comment block, a tab character counts as one space, meaning that the indentation level can drop enough to end the comment block, even if in a text editor it looks just fine. Debugging this just took me a half hour. :-)