GNU bug report logs - #57184
[PATCH] Assorted improvements to python.el

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 57184 in the body.
You can then email your comments to 57184 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Assorted improvements to python.el
Date: Sat, 13 Aug 2022 19:06:10 +0200

[Message part 1 (text/plain, inline)]

I have attached a sequence of patches with assorted small(ish)
improvements to python.el.

I hope this is a suitable format for review; let me know otherwise.

[0001-python-check-command-don-t-use-absolute-executable-n.patch (text/x-patch, attachment)]

[0002-python.el-Adjustments-to-Flymake-backend.patch (text/x-patch, attachment)]

[0003-python.el-Add-completion-predicate-symbol-property-t.patch (text/x-patch, attachment)]

[0004-python-mode-Remove-special-outline-heading-end-regex.patch (text/x-patch, attachment)]

Message #8 received at 57184 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Augusto Stoffel <arstoffel <at> gmail.com>, 57184 <at> debbugs.gnu.org
Subject: Re: bug#57184: [PATCH] Assorted improvements to python.el
Date: Sat, 13 Aug 2022 11:26:20 -0700

Augusto Stoffel <arstoffel <at> gmail.com> writes:

> I hope this is a suitable format for review; let me know otherwise.

It's perfect, thanks for the patches.  Please find some comments below.

> From c68623c2da14ae436276c84ae3705f1708e2bd92 Mon Sep 17 00:00:00 2001
> From: Augusto Stoffel <arstoffel <at> gmail.com>
> Date: Sat, 13 Aug 2022 17:04:17 +0200
> Subject: [PATCH 1/4] python-check-command: don't use absolute executable names

Better:

    python-check-command: Don't use absolute file names

> This is incompatible with Tramp and packages that switch between
> virtualenvs.

More clear:

    Absolute file names are incompatible with Tramp and packages that
    switch between virtualenvs.

> * lisp/progmodes/python.el (python-check-command): don't use absolute
> executable names.

As above, note that we prefer to capitalize "Don't".

> Subject: [PATCH 2/4] python.el: Adjustments to Flymake back-end

How about:

    python.el: Support pyflakes 2.4+ in flymake

> * lisp/progmodes/python (python-flymake-command): Advertise possiblity
> to use pylint.
> (python-flymake-command-output-pattern): Make compatible with pyflakes
> version 2.4.0
               ^^^ Missing period

> @@ -5635,7 +5639,7 @@ python-flymake-command
>  ;; TYPE
>  (defcustom python-flymake-command-output-pattern
>    (list
> -   "^\\(?:<?stdin>?\\):\\(?1:[0-9]+\\):\\(?:\\(?2:[0-9]+\\):\\)? \\(?3:.*\\)$"
> +   "^\\(?:<?stdin>?\\):\\(?1:[0-9]+\\):\\(?:\\(?2:[0-9]+\\):?\\)? \\(?3:.*\\)$"

Maybe include an example of the old/new pattern in comments?  Or even
better actually: add a test for it.

> From 95675a3f63bc420813992d7b599e99b303e4fd63 Mon Sep 17 00:00:00 2001
> From: Augusto Stoffel <arstoffel <at> gmail.com>
> Date: Sat, 13 Aug 2022 18:10:14 +0200
> Subject: [PATCH 3/4] python.el: Add completion-predicate symbol property to
>  commands
>
> * lisp/progmodes/python.el: Add a completion-predicate property to
> most commands defined in this file; some are only useful in
> python-mode, others in inferior-python mode as well.
> (python-skeleton-define, python-define-auxiliary-skeleton): Add
> appropriate completion-predicate properties.

This makes sense, but how about just creating a new mode
python-common-mode that both python-mode and inferior-python-mode could
inherit from?  Would that simplify things here?

> From 97895f6c5afcd2c0236185ff406c3d4261fbc567 Mon Sep 17 00:00:00 2001
> From: Augusto Stoffel <arstoffel <at> gmail.com>
> Date: Sat, 13 Aug 2022 18:55:48 +0200
> Subject: [PATCH 4/4] python-mode: Remove special outline-heading-end-regexp
>
> It doesn't work well with the new type annotation syntax introduced in
> Python 3.5, see bug#53913.
>
> * lisp/progmodes/python.el (python-mode): remove buffer-local setting
                                            ^^^ Capitalize "Remove"

> of outline-heading-end-regexp.
> ---
>  lisp/progmodes/python.el | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
> index 163bae07c2..8cd654ce14 100644
> --- a/lisp/progmodes/python.el
> +++ b/lisp/progmodes/python.el
> @@ -5854,7 +5854,6 @@ python-mode
>       nil))
>
>    (setq-local outline-regexp (python-rx (* space) block-start))
> -  (setq-local outline-heading-end-regexp ":[^\n]*\n")
>    (setq-local outline-level
>                (lambda ()
>                  "`outline-level' function for Python mode."

I'm fine with this as `outline-heading-end-regexp' doesn't seem to see
much use in any case (and folding to one line somehow seems nicer
anyways).

However, it seems like we might be able to do find a fix here with just
a small number of assumptions, if we really wanted to.

Message #11 received at 57184 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 57184 <at> debbugs.gnu.org
Subject: Re: bug#57184: [PATCH] Assorted improvements to python.el
Date: Sun, 14 Aug 2022 10:42:11 +0200

[Message part 1 (text/plain, inline)]

Hi Stefan,

I've attached new patches, see below for comments on your comments.  (As
to the smaller style changes, feel free to copyedit my patches before
merging in case I still let something slip.)

[0001-python-check-command-Don-t-use-absolute-file-names.patch (text/x-patch, attachment)]

[0002-python.el-Adjustments-to-Flymake-backend.patch (text/x-patch, attachment)]

[0003-python.el-Add-completion-predicate-symbol-property-t.patch (text/x-patch, attachment)]

[0004-python-mode-Remove-special-outline-heading-end-regex.patch (text/x-patch, attachment)]

[Message part 6 (text/plain, inline)]

On Sat, 13 Aug 2022 at 11:26, Stefan Kangas <stefankangas <at> gmail.com> wrote:

>> Subject: [PATCH 2/4] python.el: Adjustments to Flymake back-end
>
> How about:
>
>     python.el: Support pyflakes 2.4+ in flymake
>

This commit includes a second change advertising the possibility to use
the pylint program.  Moreover, I'm not claiming that pyflakes changed in
version 2.4; the change probably happened further in the past.  So I
made this statement less specific.

(BTW, instead of writing things like "To use `flake8' you would set this
to ..." in a docstring, wouldn't it make sense for describe-variable to
list the suggested customization values from the :type specification?)

> Maybe include an example of the old/new pattern in comments?  Or even
> better actually: add a test for it.

I've added some tests.

>> From 95675a3f63bc420813992d7b599e99b303e4fd63 Mon Sep 17 00:00:00 2001
>> From: Augusto Stoffel <arstoffel <at> gmail.com>
>> Date: Sat, 13 Aug 2022 18:10:14 +0200
>> Subject: [PATCH 3/4] python.el: Add completion-predicate symbol property to
>>  commands
>>
>> * lisp/progmodes/python.el: Add a completion-predicate property to
>> most commands defined in this file; some are only useful in
>> python-mode, others in inferior-python mode as well.
>> (python-skeleton-define, python-define-auxiliary-skeleton): Add
>> appropriate completion-predicate properties.
>
> This makes sense, but how about just creating a new mode
> python-common-mode that both python-mode and inferior-python-mode could
> inherit from?  Would that simplify things here?

In any case there are two cases to consider anyway: commands that only
make sense in Python source code, and commands that make sense in code
or in the REPL (we might eventually have code that makes sense just in
the REPL, that's not the case now).

So for the current purposes this suggestion wouldn't really simplify
anything.

> I'm fine with this as `outline-heading-end-regexp' doesn't seem to see
> much use in any case (and folding to one line somehow seems nicer
> anyways).
>
> However, it seems like we might be able to do find a fix here with just
> a small number of assumptions, if we really wanted to.

I guess the fix would be not to look for the next colon, but instead to
the next colon skipping over pairs of delimiters.  But then we couldn't
use regexps, and moreover things wouldn't work on incomplete code during
editing.

And I also think folding on one line makes just as much sense, if it
isn't better.  So it looks like removing this is the best option.

Message #14 received at 57184 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 57184 <at> debbugs.gnu.org
Subject: Re: bug#57184: [PATCH] Assorted improvements to python.el
Date: Fri, 19 Aug 2022 06:25:41 -0700

Augusto Stoffel <arstoffel <at> gmail.com> writes:

> I've attached new patches, see below for comments on your comments.  (As
> to the smaller style changes, feel free to copyedit my patches before
> merging in case I still let something slip.)

Thanks!  Patches 1-2 and 4 LGTM, so I've pushed them to master.

Patch 3 seems to break "make bootstrap" with:

    In toplevel form:
    cedet/semantic/wisent/python.el:30:2: Error: Symbol’s function
definition is void: closure
    make[3]: *** [Makefile:321: cedet/semantic/wisent/python.elc] Error 1

    In toplevel form:
    org/ob-python.el:33:2: Error: Symbol’s function definition is void: closure
    make[3]: *** [Makefile:321: org/ob-python.elc] Error 1

Could you take a look at it?

Message #17 received at 57184 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 57184 <at> debbugs.gnu.org
Subject: Re: bug#57184: [PATCH] Assorted improvements to python.el
Date: Fri, 19 Aug 2022 06:39:58 -0700

close 57184 29.1
thanks

> Patch 3 seems to break "make bootstrap" with:
>
>     In toplevel form:
>     cedet/semantic/wisent/python.el:30:2: Error: Symbol’s function
> definition is void: closure
>     make[3]: *** [Makefile:321: cedet/semantic/wisent/python.elc] Error 1
>
>     In toplevel form:
>     org/ob-python.el:33:2: Error: Symbol’s function definition is void: closure
>     make[3]: *** [Makefile:321: org/ob-python.elc] Error 1
>
> Could you take a look at it?

I fixed it by changing the completion-predicate lambda into a defun.
Now pushed to master with that change.  Thanks!

Message #22 received at 57184 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 57184 <at> debbugs.gnu.org
Subject: Re: bug#57184: [PATCH] Assorted improvements to python.el
Date: Mon, 22 Aug 2022 18:24:57 +0200

[Message part 1 (text/plain, inline)]

On Fri, 19 Aug 2022 at 06:39, Stefan Kangas <stefankangas <at> gmail.com> wrote:

>
> close 57184 29.1
> thanks
>
>> Patch 3 seems to break "make bootstrap" with:
>>
>>     In toplevel form:
>>     cedet/semantic/wisent/python.el:30:2: Error: Symbol’s function
>> definition is void: closure
>>     make[3]: *** [Makefile:321: cedet/semantic/wisent/python.elc] Error 1
>>
>>     In toplevel form:
>>     org/ob-python.el:33:2: Error: Symbol’s function definition is void: closure
>>     make[3]: *** [Makefile:321: org/ob-python.elc] Error 1
>>
>> Could you take a look at it?
>
> I fixed it by changing the completion-predicate lambda into a defun.
> Now pushed to master with that change.  Thanks!

Thanks for looking into this!

So the compilation problem seems to be caused by lambda forms that are
evaluated at the time the file is loaded.  I'd have slight preference
for keeping the lambdas, among other things because they are kind of a
provisional solution and could be deleted in a decade or so when we can
rely on the (interactive ARG . MODES) feature.

So I wonder -- why did we get those compilation errors?  Sounds like a
bug somewhere else to me.

Also, a slight fix is needed, see attached patch.

[0001-Fix-completion-predicate-of-Python-shell-commands.patch (text/x-patch, attachment)]

Message #25 received at 57184 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 57184 <at> debbugs.gnu.org
Subject: Re: bug#57184: [PATCH] Assorted improvements to python.el
Date: Mon, 22 Aug 2022 20:20:28 -0700

Augusto Stoffel <arstoffel <at> gmail.com> writes:

> So the compilation problem seems to be caused by lambda forms that are
> evaluated at the time the file is loaded.  I'd have slight preference
> for keeping the lambdas, among other things because they are kind of a
> provisional solution and could be deleted in a decade or so when we can
> rely on the (interactive ARG . MODES) feature.
>
> So I wonder -- why did we get those compilation errors?  Sounds like a
> bug somewhere else to me.

To be honest, I didn't look into it as my preference was for a function
instead of a lambda.  ;-)

But if you feel like investigating, please go ahead.  I also won't
protest if you subsequently want to get rid of the named defun and
re-introduce the lambdas.

> Also, a slight fix is needed, see attached patch.

Thanks, pushed to master (commit fd97cc8e0d).

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.