#13146 closed defect (fixed)
Removing tabs in .rst, .tex and .pxi files
Reported by: | roed | Owned by: | mvngu |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.2 |
Component: | doctest coverage | Keywords: | |
Cc: | Merged in: | sage-5.2.rc0 | |
Authors: | David Roe | Reviewers: | Keshav Kini, André Apitzsch, Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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
Attachments (2)
Change History (28)
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 follow-up: ↓ 9 Changed 9 years ago by
comment:3 Changed 9 years ago by
- Description modified (diff)
comment:4 Changed 9 years ago by
There is now a real commit message.
comment:6 Changed 9 years ago by
- Reviewers set to Keshav Kini
- Status changed from needs_review to positive_review
Cool, passes tests.
comment:7 Changed 9 years ago by
- Status changed from positive_review to needs_work
comment:8 Changed 9 years ago by
- Status changed from needs_work to needs_review
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).
comment:9 in reply to: ↑ 2 Changed 9 years ago by
- Status changed from needs_review to needs_work
Quoting kini:
Needs a real commit message, otherwise [13146_missed.patch] looks good :)
comment:11 Changed 9 years ago by
- Description modified (diff)
- Reviewers changed from Keshav Kini to Keshav Kini, André Apitzsch
- Status changed from needs_review to needs_work
You probably added by accident
-
sage/schemes/elliptic_curves/constructor.py
a b 1 1 """ 2 2 Elliptic curve constructor 3 3 4 sage: EllipticCurve([0,0]) 5 4 6 AUTHORS: 5 7 6 8 - William Stein (2005): Initial version
Please remove this.
comment:12 Changed 9 years ago by
Sorry about that. qrefresh -e committed some unintended changes.
comment:13 Changed 9 years ago by
- Status changed from needs_work to needs_review
comment:14 Changed 9 years ago by
- Status changed from needs_review to positive_review
comment:15 Changed 9 years ago by
- Milestone changed from sage-5.2 to sage-5.1
comment:16 Changed 9 years ago by
- Status changed from positive_review to needs_work
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
comment:17 Changed 9 years ago by
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?
comment:18 Changed 9 years ago by
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.
comment:19 follow-up: ↓ 21 Changed 9 years ago by
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.
comment:20 Changed 9 years ago by
Personally I'm all for getting rid of tabs everywhere. Either tabs are used semantically or not, and clearly in Sage they're not.
comment:21 in reply to: ↑ 19 Changed 9 years ago by
Replying to roed:
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.
comment:22 Changed 9 years ago by
- Status changed from needs_work to needs_review
Alright, I fixed the tabs in the Russian and German documentation.
comment:23 Changed 9 years ago by
- Reviewers changed from Keshav Kini, André Apitzsch to Keshav Kini, André Apitzsch, Jeroen Demeyer
- Status changed from needs_review to positive_review
comment:24 Changed 9 years ago by
- Milestone changed from sage-5.3 to sage-5.2
comment:25 Changed 9 years ago by
- Merged in set to sage-5.2.rc0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:26 Changed 8 years ago by
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. :-)
Needs a real commit message, otherwise looks good :)