Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#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 aapitzsch)

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)

13146_vs_51b5.patch (24.5 KB) - added by roed 7 years ago.
Changes to apply against 5.1.beta5
13146_missed.patch (5.8 KB) - added by roed 7 years ago.
Missed a few

Download all attachments as: .zip

Change History (28)

comment:1 Changed 7 years ago by roed

  • Status changed from new to needs_review

comment:2 follow-up: Changed 7 years ago by kini

Needs a real commit message, otherwise looks good :)

Changed 7 years ago by roed

Changes to apply against 5.1.beta5

comment:3 Changed 7 years ago by roed

  • Description modified (diff)

comment:4 Changed 7 years ago by roed

There is now a real commit message.

comment:5 Changed 7 years ago by kini

  • Description modified (diff)

patchbot: apply 13146_vs_51b5.patch

comment:6 Changed 7 years ago by kini

  • Reviewers set to Keshav Kini
  • Status changed from needs_review to positive_review

Cool, passes tests.

comment:7 Changed 7 years ago by roed

  • Status changed from positive_review to needs_work

comment:8 Changed 7 years ago by roed

  • 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 7 years ago by aapitzsch

  • Status changed from needs_review to needs_work

Quoting kini:

Needs a real commit message, otherwise [13146_missed.patch] looks good :)

comment:10 Changed 7 years ago by roed

  • Status changed from needs_work to needs_review

Fixed. :)

comment:11 Changed 7 years ago by aapitzsch

  • 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

Please remove this.

comment:12 Changed 7 years ago by roed

Sorry about that. qrefresh -e committed some unintended changes.

comment:13 Changed 7 years ago by roed

  • Status changed from needs_work to needs_review

comment:14 Changed 7 years ago by aapitzsch

  • Status changed from needs_review to positive_review

comment:15 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.2 to sage-5.1

comment:16 Changed 7 years ago by jdemeyer

  • 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 7 years ago by jhpalmieri

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

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: Changed 7 years ago by 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. I can change it to allow tabs if people like and make this ticket invalid.

comment:20 Changed 7 years ago by kini

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

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.

Changed 7 years ago by roed

Missed a few

comment:22 Changed 7 years ago by roed

  • Status changed from needs_work to needs_review

Alright, I fixed the tabs in the Russian and German documentation.

comment:23 Changed 7 years ago by jdemeyer

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

  • Milestone changed from sage-5.3 to sage-5.2

comment:25 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.2.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:26 Changed 7 years ago by roed

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

Note: See TracTickets for help on using tickets.