tabs in the Sage library
Dan Drake
A while back, there was a massive patch that removed all the tabs from
the Sage library. Unfortunately, they have been creeping back in, and
there are now quite a few files with tabs: try doing
grep --perl-regexp '\t' --files-with-matches -r sage/*
from SAGE_ROOT/devel/sage/.
Coordinating another big tab removal patch would be difficult, but I
think that we could at least change the "sage -merge" script so that it
rejects any patch that introduces tabs. (Unless, say, some kind of
--no-really-the-tabs-are-okay command line option is specified.) Does
this seem like a good idea?
(This is inspired by #3852, where I fixed up a bunch of tabs and
whitespace in symbolics/expression.pyx, and Burcin pointed out that such
a patch would mess up a bunch of other patches that affect that file.)
Dan
--
--- Dan Drake
----- http://mathsci.kaist.ac.kr/~drake
-------
Tom Boothby
> think that we could at least change the "sage -merge" script so that it
> rejects any patch that introduces tabs. (Unless, say, some kind of
> --no-really-the-tabs-are-okay command line option is specified.) Does
> this seem like a good idea?
+1
John H Palmieri
> Hello,
>
> A while back, there was a massive patch that removed all the tabs from
> the Sage library. Unfortunately, they have been creeping back in, and
> there are now quite a few files with tabs: try doing
>
> grep --perl-regexp '\t' --files-with-matches -r sage/*
or from sage: search_src("\t")
This is annoying; it was a pain to get rid of the tabs last time. If
I remember, having tabs in a file can cause problems with Sphinx,
maybe with displaying help in the notebook.
> Coordinating another big tab removal patch would be difficult, but I
I put together a patch in about 20 minutes, using search_src("\t") and
the emacs "untabify" function. If our next release is 4.4, then 4.4.1
could be purely an untabify release...
> think that we could at least change the "sage -merge" script so that it
> rejects any patch that introduces tabs. (Unless, say, some kind of
> --no-really-the-tabs-are-okay command line option is specified.) Does
> this seem like a good idea?
Yes. Another option: should "sage -t" fail on any file with tabs?
Then the responsibility would fall more to the patch authors than to
the release managers.
--
John
William Stein
+1 -- I greatly prefer that option. In particular, it means that a
release manager does not have to use "sage -merge" if they don't want
to. Changing sage -t is much better.
-- William
--
William Stein
Professor of Mathematics
University of Washington
http://wstein.org
John H Palmieri
> > Yes. Another option: should "sage -t" fail on any file with tabs?
> > Then the responsibility would fall more to the patch authors than to
> > the release managers.
>
> +1 -- I greatly prefer that option. In particular, it means that a
> release manager does not have to use "sage -merge" if they don't want
> to. Changing sage -t is much better.
Here's a patch:
<http://trac.sagemath.org/sage_trac/ticket/8680>
--
John
Nathan O'Treally
it would be better to have a tool that (conditionally) does this
rather than supplying patches to lots of files converted by Emacs. The
same tool could be used just to check for "illegal" tabs. As I
understand this, these are *tabs in leading whitespace*.
Trailing whitespace could be removed. (Currently trailing tabs are
converted to many trailing spaces.)
I've also come across files that do not end with newline...
Opinions?
-Leif
(see also <http://trac.sagemath.org/sage_trac/ticket/8680#comment:9>)
William Stein
> I'm not happy with "brute-force" converting *any* tab to space(s), and
Why? Is there a single place in all sage that should use a tab in the
source code? Please clarify?
> it would be better to have a tool that (conditionally) does this
> rather than supplying patches to lots of files converted by Emacs. The
> same tool could be used just to check for "illegal" tabs. As I
> understand this, these are *tabs in leading whitespace*.
>
> Trailing whitespace could be removed. (Currently trailing tabs are
> converted to many trailing spaces.)
That is another issue. It doesn't cause any trouble in python, so it
doesn't concern me. Mixing spaces and tabs does cause trouble, and I
think using tabs at all violates the style guidelines in our developer
manual.
>
> I've also come across files that do not end with newline...
>
> Opinions?
>
> -Leif
>
> (see also <http://trac.sagemath.org/sage_trac/ticket/8680#comment:9>)
>
> --
> To post to this group, send an email to sage-...@googlegroups.com
> To unsubscribe from this group, send an email to sage-devel+...@googlegroups.com
> For more options, visit this group at http://groups.google.com/group/sage-devel
> URL: http://www.sagemath.org
>
> To unsubscribe, reply using "remove me" as the subject.
Robert Bradshaw
> I'm not happy with "brute-force" converting *any* tab to space(s), and
> it would be better to have a tool that (conditionally) does this
> rather than supplying patches to lots of files converted by Emacs. The
> same tool could be used just to check for "illegal" tabs. As I
> understand this, these are *tabs in leading whitespace*.
>
> Trailing whitespace could be removed. (Currently trailing tabs are
> converted to many trailing spaces.)
>
> I've also come across files that do not end with newline...
>
> Opinions?
I think we should do this once, after a tool is in place to make sure
these things don't get in anymore.
A big +1 to sage -t failing on files that have tabs, lack newlines, or
whatever else the style guide enforces. I've also thought it would be
useful to have a trac plugin that points out such things (and it could
report coverage changes as well).
- Robert
John H Palmieri
> I'm not happy with "brute-force" converting *any* tab to space(s), and
> it would be better to have a tool that (conditionally) does this
> rather than supplying patches to lots of files converted by Emacs. The
> same tool could be used just to check for "illegal" tabs. As I
> understand this, these are *tabs in leading whitespace*.
Well, I think it's more complicated than that, or at least it used to
be: when we first started using Sphinx to produce docstrings in the
notebook, if there was a tab *anywhere* in the file, then docstrings
for that file would not be processed correctly by Sphinx. This is the
main reason that I personally wanted to get rid of all tabs in .py
and .pyx files. I haven't tested lately to see if that's still an
issue.
That is, it's not just an esthetic issue or a style issue: there is
(or was?) a very good reason to eliminate all tabs, not just in
leading whitespace.
> Trailing whitespace could be removed. (Currently trailing tabs are
> converted to many trailing spaces.)
>
> I've also come across files that do not end with newline...
I certainly don't object to dealing with these, but they don't have
the same sense of importance to me. (Or maybe they're important but I
don't know why?)
--
John
Florent Hivert
> I think we should do this once, after a tool is in place to make sure these
> things don't get in anymore.
>
> A big +1 to sage -t failing on files that have tabs, lack newlines, or
> whatever else the style guide enforces. I've also thought it would be
> useful to have a trac plugin that points out such things (and it could
> report coverage changes as well).
Whatever solution is taken, I'm quite concerned with the forthcoming nightmare
of rebasing the combinat queue while trying to keep it compatible with the
former version (currently the queue still apply on sage-4.3.1). At the very
least, the patch should be the first entering a new sage release. So that we
can apply the script on the few former version to get compatibility patch and
have a at-most-as-possible-common tab-free base for the queue.
Cheers,
Florent
Nicolas M. Thiery
> > I think we should do this once, after a tool is in place to make sure these
> > things don't get in anymore.
> >
> > A big +1 to sage -t failing on files that have tabs, lack newlines, or
> > whatever else the style guide enforces. I've also thought it would be
> > useful to have a trac plugin that points out such things (and it could
> > report coverage changes as well).
>
> I'm also +1 on these with the following reserve:
>
> Whatever solution is taken, I'm quite concerned with the forthcoming nightmare
> of rebasing the combinat queue while trying to keep it compatible with the
> former version (currently the queue still apply on sage-4.3.1). At the very
> least, the patch should be the first entering a new sage release. So that we
> can apply the script on the few former version to get compatibility patch and
> have a at-most-as-possible-common tab-free base for the queue.
them!
But please please, let's do that smoothly without conflicting with all
the current developments. To this end, I vote for sage -t to report
tabs as warnings (not failures) for the moment. This should be enough
of an incentive for the developers working in the various code areas
to fix those as a side effect of their own development. In a couple
releases, we can then switch to errors. Those remaining tabs, if any,
will then be in stable areas and can be fixed then without risks of
conflicts.
By the way, it would be nice to have sage -t also report as warnings:
- non ascii characters (unless the encoding is specified)
- trailing white spaces
Speaking of those: they sure are anesthetic and I remove them when I
spot some around lines I am anyway changing. But apart from
exceptional situations like Minh's, they seem quite harmless. So
fixing those should not cause any risk of conflict. I think a warning
would enough of an incent.
Cheers,
Nicolas
--
Nicolas M. Thiéry "Isil" <nth...@users.sf.net>
http://Nicolas.Thiery.name/
John H Palmieri
wrote:
> But please please, let's do that smoothly without conflicting with all
> the current developments. To this end, I vote for sage -t to report
> tabs as warnings (not failures) for the moment.
That is, it's easy to print out a warning if there is a tab in a file
when the file is doctested, but if you doctest the whole Sage library,
printing out warnings at the end of all tests (separately, and in
addition to, failures) might be tricky. But I think you have to have
some kind of summary at the end because it would be too easy to miss a
few warnings among the hundreds of files being tested.
I could be wrong about it being harder to implement...
> By the way, it would be nice to have sage -t also report as warnings:
>
> - non ascii characters (unless the encoding is specified)
> - trailing white spaces
--
John
Nicolas M. Thiery
> I think this is a good idea, but might be a bit harder to implement.
> That is, it's easy to print out a warning if there is a tab in a file
> when the file is doctested, but if you doctest the whole Sage library,
> printing out warnings at the end of all tests (separately, and in
> addition to, failures) might be tricky. But I think you have to have
> some kind of summary at the end because it would be too easy to miss a
> few warnings among the hundreds of files being tested.
given file to fix (or at least not add) tabs in that file. It seems a
reasonable assumption that those developers will run tests on just
that file (or just a few files around) on a regular basis, and then
will spot the warning.