bug#64406: [PATCH] Improve commands to manage Python imports

bug#64406: [PATCH] Improve commands to manage Python imports

[Matthias Meulien]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]


Commands python-add-import, python-remove-imports and
python-sort-imports run a short Python script stored in variable
python--list-imports; That script starts with:

    from isort import find_imports_in_stream, find_imports_in_paths

But find_imports_in_stream and find_imports_in_paths were introduced in
version 5.7.0 of the isort library.

Debian bookworm has isort version 5.6.4 in its package repositories.
Thus on such system commands python-add-import, python-remove-import
and python-sort-imports fail with error message:

    python exited with status 1 (maybe isort is missing?)

This is misleading when the isort library is installed.

I suggest to extend the Python script in order to catch the import
errors and return status 1 if the isort library is not found and status
2 if the expected symbols aren't found.  The error message can then be
specialized.  This is what I implemented in the attached patch.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-Python-imports-management-commands.patch --]
[-- Type: text/x-diff, Size: 2385 bytes --]

From a667c1f6f5a62a570f70d849c92076966c41364b Mon Sep 17 00:00:00 2001
From: Matthias Meulien <orontee@gmail.com>
Date: Sat, 1 Jul 2023 22:12:43 +0200
Subject: [PATCH] Improve Python imports management commands

* lisp/progmodes/python.el (python--list-imports): Handle import errors.
(python--do-isort): Specialize error message.
---
 lisp/progmodes/python.el | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 50d712ebb0c..4291ab03ca6 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -6446,8 +6446,14 @@ python-flymake
 \f
 ;;; Import management
 (defconst python--list-imports "\
-from isort import find_imports_in_stream, find_imports_in_paths
-from sys import argv, stdin
+from sys import argv, exit, stdin
+
+try:
+    from isort import find_imports_in_stream, find_imports_in_paths
+except ModuleNotFoundError:
+    exit(1)
+except ImportError:
+    exit(2)
 
 query, files, result = argv[1] or None, argv[2:], {}
 
@@ -6501,9 +6507,13 @@ python--list-imports
                                 (or name "")
                                 (mapcar #'file-local-name source)))))
              lines)
-        (unless (eq 0 status)
+        (cond
+         ((eq 1 status)
           (error "%s exited with status %s (maybe isort is missing?)"
                  python-interpreter status))
+         ((eq 2 status)
+          (error "%s exited with status %s (maybe isort version is <5.7.0?)"
+                 python-interpreter status)))
         (goto-char (point-min))
         (while (not (eobp))
 	  (push (buffer-substring-no-properties (point) (pos-eol))
@@ -6546,9 +6556,13 @@ python--do-isort
                                nil (list temp nil) nil
                                "-m" "isort" "-" args))
                 (tick (buffer-chars-modified-tick)))
-            (unless (eq 0 status)
+            (cond
+             ((eq 1 status)
               (error "%s exited with status %s (maybe isort is missing?)"
                      python-interpreter status))
+             ((eq 2 status)
+              (error "%s exited with status %s (maybe isort version is <5.7.0?)"
+                     python-interpreter status)))
             (replace-buffer-contents temp)
             (not (eq tick (buffer-chars-modified-tick)))))))))
 
-- 
2.39.2


[-- Attachment #3: Type: text/plain, Size: 8753 bytes --]






In GNU Emacs 29.0.91 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.37, cairo version 1.16.0) of 2023-06-13 built on carbon
Repository revision: 212884f2bfed7f00e58aad183edd20ecc2a23e71
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12101007
System Description: Debian GNU/Linux 12 (bookworm)

Configured using:
 'configure --with-json --with-native-compilation --with-x-toolkit=gtk
 --with-tree-sitter'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3
THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER WEBP X11 XDBE XIM XINPUT2
XPM GTK3 ZLIB

Important settings:
  value of $LANG: fr_FR.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Shell

Minor modes in effect:
  shell-dirtrack-mode: t
  goto-address-mode: t
  highlight-changes-visible-mode: t
  breadcrumb-mode: t
  minions-mode: t
  global-company-mode: t
  company-mode: t
  comint-fontify-input-mode: t
  desktop-save-mode: t
  server-mode: t
  pixel-scroll-precision-mode: t
  save-place-mode: t
  electric-pair-mode: t
  icomplete-mode: t
  global-so-long-mode: t
  global-auto-revert-mode: t
  auto-insert-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-layout-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tab-bar-mode: t
  file-name-shadow-mode: t
  context-menu-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  window-divider-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/home/matthias/.config/emacs/elpa/jsonrpc-1.0.17/jsonrpc hides /usr/local/share/emacs/29.0.91/lisp/jsonrpc
/home/matthias/.config/emacs/elpa/flymake-1.3.4/flymake hides /usr/local/share/emacs/29.0.91/lisp/progmodes/flymake
/home/matthias/.config/emacs/elpa/project-0.9.8/project hides /usr/local/share/emacs/29.0.91/lisp/progmodes/project
/home/matthias/.config/emacs/elpa/eglot-1.15/eglot hides /usr/local/share/emacs/29.0.91/lisp/progmodes/eglot
/home/matthias/.config/emacs/elpa/dictionary-20201001.1727/dictionary hides /usr/local/share/emacs/29.0.91/lisp/net/dictionary
/home/matthias/.config/emacs/elpa/soap-client-3.2.3/soap-client hides /usr/local/share/emacs/29.0.91/lisp/net/soap-client
/home/matthias/.config/emacs/elpa/soap-client-3.2.3/soap-inspect hides /usr/local/share/emacs/29.0.91/lisp/net/soap-inspect
/home/matthias/.config/emacs/elpa/eldoc-1.14.0/eldoc hides /usr/local/share/emacs/29.0.91/lisp/emacs-lisp/eldoc

Features:
(ediff ediff-merg ediff-mult ediff-wind ediff-diff ediff-help ediff-init
ediff-util view grep tramp-archive tramp-gvfs tramp-cache time-stamp
zeroconf tramp tramp-loaddefs trampver tramp-integration tramp-compat
ls-lisp smiley gnus-async gnus-bcklg gnus-ml disp-table nndoc gnus-dup
mm-archive url-cache crm debbugs-gnu debbugs-compat debbugs soap-client
url-http url-auth url-gw log-edit vc-annotate emacs-news-mode pcmpl-git
whitespace dash pcmpl-unix pcmpl-gnu edebug pulse bug-reference
mailalias smtpmail textsec uni-scripts idna-mapping ucs-normalize
uni-confusable textsec-check shadow sort gnus-cite mail-extr emacsbug
gnus-topic nndraft nnmh nnfolder utf-7 epa-file network-stream nsm
gnus-agent gnus-srvr gnus-score score-mode nnvirtual gnus-msg gnus-cache
dabbrev shortdoc tabify cus-start cl-print help-fns radix-tree
mhtml-mode hideshow cap-words superword subword css-mode js c-ts-common
make-mode follow mule-util yaml-mode rst smerge-mode diff rng-xsd
xsd-regexp rng-cmpct rng-nxml rng-valid nxml-mode nxml-outln nxml-rap
sgml-mode facemenu add-log sh-script smie executable files-x shell vc-hg
vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs log-view pcvs-util misearch
multi-isearch image-dired image-dired-tags image-dired-external
image-dired-util man hl-line dired-aux conf-mode flyspell ox-odt rng-loc
rng-uri rng-parse rng-match rng-dt rng-util rng-pttrn nxml-parse nxml-ns
nxml-enc xmltok nxml-util ox-latex ox-icalendar org-agenda ox-html table
ox-ascii ox-publish ox oc-basic org-element org-persist org-id avl-tree
ol-eww eww xdg url-queue mm-url ol-rmail ol-mhe ol-irc ol-info ol-gnus
nnselect gnus-art mm-uu mml2015 mm-view mml-smime smime gnutls dig
gnus-sum shr pixel-fill kinsoku url-file svg dom ol-docview doc-view
image-mode exif ol-bibtex bibtex ol-bbdb ol-w3m ol-doi org-link-doi
vc-dir vc goto-addr jka-compr display-line-numbers hilit-chg eglot
external-completion jsonrpc flymake-proc flymake ert ewoc debug
backtrace compile breadcrumb imenu company-oddmuse company-keywords
company-etags etags fileloop generator xref company-gtags
company-dabbrev-code company-dabbrev company-files company-clang
company-capf company-cmake company-semantic company-template
company-bbdb ob-python python project treesit minions compat company
pcase carbon-custom cus-edit cus-load gnus-demon nntp gnus-group
gnus-undo gnus-start gnus-dbus dbus xml gnus-cloud nnimap nnmail
mail-source utf7 parse-time iso8601 gnus-spec gnus-win nnoo gnus-int
gnus-range message sendmail yank-media puny rfc822 mml mml-sec epa
derived epg rfc6068 epg-config mm-decode mm-bodies mm-encode mail-parse
rfc2231 rfc2047 rfc2045 ietf-drums mailabbrev gmm-utils mailheader gnus
nnheader gnus-util mail-utils range mm-util mail-prsvr wid-edit
gnus-dired dired-x dired dired-loaddefs org-capture org-refile org ob
ob-tangle ob-ref ob-lob ob-table ob-exp org-macro org-src ob-comint
org-pcomplete pcomplete comint ansi-osc ansi-color org-list org-footnote
org-faces org-entities time-date ob-emacs-lisp ob-core ob-eval org-cycle
org-table ol org-fold org-fold-core org-keys oc org-loaddefs find-func
cal-menu calendar cal-loaddefs org-compat org-version org-macs
format-spec dictionary link connection advice markdown-mode color
thingatpt noutline outline skeleton find-file vc-git diff-mode
easy-mmode vc-dispatcher ispell comp comp-cstr warnings icons rx
cl-extra help-mode desktop frameset server bookmark text-property-search
pp pixel-scroll cua-base ring saveplace elec-pair icomplete so-long
autorevert filenotify autoinsert cc-mode cc-fonts cc-guess cc-menus
cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs generic-x
face-remap yasnippet-snippets-autoloads git-link-autoloads
flymake-easy-autoloads proof-general-autoloads proof-site
proof-autoloads debbugs-autoloads minions-autoloads compat-autoloads
markdown-mode-autoloads typescript-mode-autoloads soap-client-autoloads
breadcrumb-autoloads yaml-mode-autoloads dockerfile-mode-autoloads
terraform-mode-autoloads hcl-mode-autoloads restclient-jq-autoloads
restclient-autoloads dash-autoloads eglot-autoloads jsonrpc-autoloads
company-autoloads caml-autoloads jq-mode-autoloads csv-mode-autoloads
info mmm-mode-autoloads yaml-autoloads rfc-mode-autoloads
mallard-mode-autoloads go-mode-autoloads flymake-autoloads
project-autoloads eldoc-autoloads package browse-url url url-proxy
url-privacy url-expand url-methods url-history url-cookie
generate-lisp-file url-domsuf url-util mailcap url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs password-cache json subr-x
map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs cl-lib rmc
iso-transl tooltip cconv eldoc paren electric uniquify ediff-hook
vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo move-toolbar gtk x-toolkit xinput2 x multi-tty
make-network-process native-compile emacs)

Memory information:
((conses 16 2066221 235411)
 (symbols 48 46948 48)
 (strings 32 296918 49042)
 (string-bytes 1 10865324)
 (vectors 16 131994)
 (vector-slots 8 3068685 400863)
 (floats 8 3536 1265)
 (intervals 56 113296 13856)
 (buffers 984 169))

bug#64406: [PATCH] Improve commands to manage Python imports

[Eli Zaretskii]

> From: Matthias Meulien <orontee@gmail.com>
> Date: Sat, 01 Jul 2023 22:16:28 +0200
> 
> Commands python-add-import, python-remove-imports and
> python-sort-imports run a short Python script stored in variable
> python--list-imports; That script starts with:
> 
>     from isort import find_imports_in_stream, find_imports_in_paths
> 
> But find_imports_in_stream and find_imports_in_paths were introduced in
> version 5.7.0 of the isort library.
> 
> Debian bookworm has isort version 5.6.4 in its package repositories.
> Thus on such system commands python-add-import, python-remove-import
> and python-sort-imports fail with error message:
> 
>     python exited with status 1 (maybe isort is missing?)
> 
> This is misleading when the isort library is installed.
> 
> I suggest to extend the Python script in order to catch the import
> errors and return status 1 if the isort library is not found and status
> 2 if the expected symbols aren't found.  The error message can then be
> specialized.  This is what I implemented in the attached patch.

Thanks, installed on the master branch and closing the bug.

bug#64406: [PATCH] Improve commands to manage Python imports

[Augusto Stoffel]

On Sat,  1 Jul 2023 at 22:16, Matthias Meulien wrote:

> -from sys import argv, stdin
> +from sys import argv, exit, stdin

FWIW, exit is already in the global namespace.

> -        (unless (eq 0 status)
> +        (cond
> +         ((eq 1 status)
>            (error "%s exited with status %s (maybe isort is missing?)"
>                   python-interpreter status))
> +         ((eq 2 status)
> +          (error "%s exited with status %s (maybe isort version is <5.7.0?)"
> +                 python-interpreter status)))

This change implies that the "success" branch may run if the exit code
is nonzero (without knowing all the isort internals, it can't be
excluded that an exit code > 2 is used somewhere).  I suggest instead a
(pcase status ...) to construct the " (maybe... ?)" segment of the error
message.

We could also take this opportunity to distinguish between random
exceptions happening in the script (which likely leads to exit code 1)
and the ModuleNotFoundError case.

bug#64406: [PATCH] Improve commands to manage Python imports

[Matthias Meulien]

Augusto Stoffel <arstoffel@gmail.com> writes:

> On Sat,  1 Jul 2023 at 22:16, Matthias Meulien wrote:
>
>> -from sys import argv, stdin
>> +from sys import argv, exit, stdin
>
> FWIW, exit is already in the global namespace.

Yes but it isn't meant for use in programs; The documentation says:

    The *note site module (which is imported automatically during startup,
    except if the -S command-line option is given) adds several
    constants to the built-in namespace.  They are useful for the
    interactive interpreter shell and should not be used in programs.


>> -        (unless (eq 0 status)
>> +        (cond
>> +         ((eq 1 status)
>>            (error "%s exited with status %s (maybe isort is missing?)"
>>                   python-interpreter status))
>> +         ((eq 2 status)
>> +          (error "%s exited with status %s (maybe isort version is <5.7.0?)"
>> +                 python-interpreter status)))
>
> This change implies that the "success" branch may run if the exit code
> is nonzero (without knowing all the isort internals, it can't be
> excluded that an exit code > 2 is used somewhere).  I suggest instead a
> (pcase status ...) to construct the " (maybe... ?)" segment of the error
> message.

Thanks, I'll fix this.

> We could also take this opportunity to distinguish between random
> exceptions happening in the script (which likely leads to exit code 1)
> and the ModuleNotFoundError case.

Good point, I'll improve this too.
-- 
Matthias

bug#64406: [PATCH] Improve commands to manage Python imports

[Matthias Meulien]

[-- Attachment #1: Type: text/plain, Size: 959 bytes --]

Matthias Meulien <orontee@gmail.com> writes:

> Augusto Stoffel <arstoffel@gmail.com> writes:
>> (...)
>> This change implies that the "success" branch may run if the exit code
>> is nonzero (without knowing all the isort internals, it can't be
>> excluded that an exit code > 2 is used somewhere).  I suggest instead a
>> (pcase status ...) to construct the " (maybe... ?)" segment of the error
>> message.
>
> Thanks, I'll fix this.

>> We could also take this opportunity to distinguish between random
>> exceptions happening in the script (which likely leads to exit code 1)
>> and the ModuleNotFoundError case.
>
> Good point, I'll improve this too.

Here are two patches.

The first one revert part of the original patch where I introduced a
useless detailed check of isort exit status itself.

The second one relates to your remarks: It shift the custom exit status
of the Python script and make sure that an error is reported assoon as
exit code >0.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-Improve-Python-imports-management-commands.patch --]
[-- Type: text/x-diff, Size: 1276 bytes --]

From 2463a33c5c3b8eb38da8f064459900159330bb47 Mon Sep 17 00:00:00 2001
From: Matthias Meulien <orontee@gmail.com>
Date: Sun, 9 Jul 2023 07:45:57 +0200
Subject: [PATCH 1/2] Fix "Improve Python imports management commands"

* lisp/progmodes/python.el (python--do-isort): The isort library don't
check for its version itself!
---
 lisp/progmodes/python.el | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 4291ab03ca6..8ce10621ed3 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -6556,13 +6556,9 @@ python--do-isort
                                nil (list temp nil) nil
                                "-m" "isort" "-" args))
                 (tick (buffer-chars-modified-tick)))
-            (cond
-             ((eq 1 status)
+            (unless (eq 0 status)
               (error "%s exited with status %s (maybe isort is missing?)"
                      python-interpreter status))
-             ((eq 2 status)
-              (error "%s exited with status %s (maybe isort version is <5.7.0?)"
-                     python-interpreter status)))
             (replace-buffer-contents temp)
             (not (eq tick (buffer-chars-modified-tick)))))))))
 
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Improve-Python-imports-management-commands.patch --]
[-- Type: text/x-diff, Size: 2282 bytes --]

From 65e5229e62fffc96251e28b90a56c8d79c53703c Mon Sep 17 00:00:00 2001
From: Matthias Meulien <orontee@gmail.com>
Date: Sun, 9 Jul 2023 07:48:57 +0200
Subject: [PATCH 2/2] Improve Python imports management commands

* lisp/progmodes/python.el (python--list-imports): Prefer to use an
exit status >1.
(python--list-imports-check-status): Check Python script status.
---
 lisp/progmodes/python.el | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 8ce10621ed3..ea68d7ee59d 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -6451,9 +6451,9 @@ python--list-imports
 try:
     from isort import find_imports_in_stream, find_imports_in_paths
 except ModuleNotFoundError:
-    exit(1)
-except ImportError:
     exit(2)
+except ImportError:
+    exit(3)
 
 query, files, result = argv[1] or None, argv[2:], {}
 
@@ -6484,6 +6484,17 @@ python--import-sources
                   (project-files proj))
     (list default-directory)))
 
+(defun python--list-imports-check-status (status)
+  (unless (eq 0 status)
+    (let* ((details
+            (cond
+             ((eq 2 status) " (maybe isort is missing?)")
+             ((eq 3 status) " (maybe isort version is less than 5.7.0?)")
+             (t "")))
+           (msg
+            (concat "%s exited with status %s" details)))
+      (error msg python-interpreter status))))
+
 (defun python--list-imports (name source)
   "List all Python imports matching NAME in SOURCE.
 If NAME is nil, list all imports.  SOURCE can be a buffer or a
@@ -6507,13 +6518,7 @@ python--list-imports
                                 (or name "")
                                 (mapcar #'file-local-name source)))))
              lines)
-        (cond
-         ((eq 1 status)
-          (error "%s exited with status %s (maybe isort is missing?)"
-                 python-interpreter status))
-         ((eq 2 status)
-          (error "%s exited with status %s (maybe isort version is <5.7.0?)"
-                 python-interpreter status)))
+        (python--list-imports-check-status status)
         (goto-char (point-min))
         (while (not (eobp))
 	  (push (buffer-substring-no-properties (point) (pos-eol))
-- 
2.39.2

bug#64406: [PATCH] Improve commands to manage Python imports

[Matthias Meulien]

[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]

Sorry Eli for asking such trivial question but I didn't manage to find
hints on the web: Now that this bug is tagged as done, should I create a
new bug for last two patches or is it possible to reopen?

Le dim. 9 juil. 2023 à 08:09, Matthias Meulien <orontee@gmail.com> a écrit :

> Matthias Meulien <orontee@gmail.com> writes:
>
> > Augusto Stoffel <arstoffel@gmail.com> writes:
> >> (...)
> >> This change implies that the "success" branch may run if the exit code
> >> is nonzero (without knowing all the isort internals, it can't be
> >> excluded that an exit code > 2 is used somewhere).  I suggest instead a
> >> (pcase status ...) to construct the " (maybe... ?)" segment of the error
> >> message.
> >
> > Thanks, I'll fix this.
>
> >> We could also take this opportunity to distinguish between random
> >> exceptions happening in the script (which likely leads to exit code 1)
> >> and the ModuleNotFoundError case.
> >
> > Good point, I'll improve this too.
>
> Here are two patches.
>
> The first one revert part of the original patch where I introduced a
> useless detailed check of isort exit status itself.
>
> The second one relates to your remarks: It shift the custom exit status
> of the Python script and make sure that an error is reported assoon as
> exit code >0.
>
>

-- 
Matthias

[-- Attachment #2: Type: text/html, Size: 2021 bytes --]

bug#64406: [PATCH] Improve commands to manage Python imports

[Eli Zaretskii]

> From: Matthias Meulien <orontee@gmail.com>
> Date: Sun, 9 Jul 2023 08:32:09 +0200
> Cc: 64406@debbugs.gnu.org, Augusto Stoffel <arstoffel@gmail.com>
> 
> Sorry Eli for asking such trivial question but I didn't manage to find hints on the web: Now that this bug
> is tagged as done, should I create a new bug for last two patches or is it possible to reopen?

No need to submit a new bug, if the discussion is still about the same
issue.  Reopening is appropriate if the original bug is deemed not
fixed; otherwise, just keep CC'ing the bug number, even so it is
closed.

IOW, "closed" doesn't mean the discussion cannot go on, and also
doesn't mean we cannot augment the solution by additional fixes for
whatever leftovers we find.

Did you read admin/notes/bugtracker?  It might have cleared up some of
this.  It also explains how to reopen a bug if you need that.

bug#64406: [PATCH] Improve commands to manage Python imports

[Matthias Meulien]

Eli Zaretskii <eliz@gnu.org> writes:

> (...)  No need to submit a new bug, if the discussion is still about
> the same issue.  Reopening is appropriate if the original bug is
> deemed not fixed; otherwise, just keep CC'ing the bug number, even so
> it is closed.
>
> IOW, "closed" doesn't mean the discussion cannot go on, and also
> doesn't mean we cannot augment the solution by additional fixes for
> whatever leftovers we find.

Ok, I understand.

> Did you read admin/notes/bugtracker?  It might have cleared up some of
> this.  It also explains how to reopen a bug if you need that.

No, I hadn't read that file.  Thanks for the reference.  
-- 
Matthias

bug#64406: [PATCH] Improve commands to manage Python imports

[Eli Zaretskii]

> Cc: 64406@debbugs.gnu.org
> From: Matthias Meulien <orontee@gmail.com>
> Date: Sun, 09 Jul 2023 08:09:17 +0200
> 
> Here are two patches.
> 
> The first one revert part of the original patch where I introduced a
> useless detailed check of isort exit status itself.
> 
> The second one relates to your remarks: It shift the custom exit status
> of the Python script and make sure that an error is reported assoon as
> exit code >0.

Thanks.  Could you please make a single patch from these two?  There's
no need to revert the original one in a separate patch.

> Subject: [PATCH 1/2] Fix "Improve Python imports management commands"
> 
> * lisp/progmodes/python.el (python--do-isort): The isort library don't
> check for its version itself!

I don't think I understand this log entry.  What does it mean "the
isort library don't check its version"?
> +             ((eq 3 status) " (maybe isort version is less than 5.7.0?)")
                                                      ^^^^^^^
Here, "is older" should be more clear, I think.

bug#64406: [PATCH] Improve commands to manage Python imports

[Matthias Meulien]

[-- Attachment #1: Type: text/plain, Size: 1159 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 64406@debbugs.gnu.org
>> From: Matthias Meulien <orontee@gmail.com>
>> Date: Sun, 09 Jul 2023 08:09:17 +0200
>> 
>> Here are two patches.
>> 
>> The first one revert part of the original patch where I introduced a
>> useless detailed check of isort exit status itself.
>> 
>> The second one relates to your remarks: It shift the custom exit status
>> of the Python script and make sure that an error is reported assoon as
>> exit code >0.
>
> Thanks.  Could you please make a single patch from these two?  There's
> no need to revert the original one in a separate patch.

Yes.

>> Subject: [PATCH 1/2] Fix "Improve Python imports management commands"
>> 
>> * lisp/progmodes/python.el (python--do-isort): The isort library don't
>> check for its version itself!
>
> I don't think I understand this log entry.  What does it mean "the
> isort library don't check its version"?

You're right, it doesn't make sense.


>> +             ((eq 3 status) " (maybe isort version is less than 5.7.0?)")
>                                                       ^^^^^^^
> Here, "is older" should be more clear, I think.

Ok.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-Improve-Python-imports-management-commands.patch --]
[-- Type: text/x-diff, Size: 3070 bytes --]

From 778324822a9e191e33e6890555d65d3500a7109c Mon Sep 17 00:00:00 2001
From: Matthias Meulien <orontee@gmail.com>
Date: Thu, 13 Jul 2023 22:47:01 +0200
Subject: [PATCH] Fix "Improve Python imports management commands"

* lisp/progmodes/python.el (python--list-imports): Prefer to use an
exit status >1.
(python--list-imports-check-status): New function to check status of
Python script.
(python--do-isort): Fix wrong status check introduced with
6295d7abdd4.
---
 lisp/progmodes/python.el | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 4291ab03ca6..a23339a2180 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -6451,9 +6451,9 @@ python--list-imports
 try:
     from isort import find_imports_in_stream, find_imports_in_paths
 except ModuleNotFoundError:
-    exit(1)
-except ImportError:
     exit(2)
+except ImportError:
+    exit(3)
 
 query, files, result = argv[1] or None, argv[2:], {}
 
@@ -6484,6 +6484,17 @@ python--import-sources
                   (project-files proj))
     (list default-directory)))
 
+(defun python--list-imports-check-status (status)
+  (unless (eq 0 status)
+    (let* ((details
+            (cond
+             ((eq 2 status) " (maybe isort is missing?)")
+             ((eq 3 status) " (maybe isort version is older than 5.7.0?)")
+             (t "")))
+           (msg
+            (concat "%s exited with status %s" details)))
+      (error msg python-interpreter status))))
+
 (defun python--list-imports (name source)
   "List all Python imports matching NAME in SOURCE.
 If NAME is nil, list all imports.  SOURCE can be a buffer or a
@@ -6507,13 +6518,7 @@ python--list-imports
                                 (or name "")
                                 (mapcar #'file-local-name source)))))
              lines)
-        (cond
-         ((eq 1 status)
-          (error "%s exited with status %s (maybe isort is missing?)"
-                 python-interpreter status))
-         ((eq 2 status)
-          (error "%s exited with status %s (maybe isort version is <5.7.0?)"
-                 python-interpreter status)))
+        (python--list-imports-check-status status)
         (goto-char (point-min))
         (while (not (eobp))
 	  (push (buffer-substring-no-properties (point) (pos-eol))
@@ -6556,13 +6561,9 @@ python--do-isort
                                nil (list temp nil) nil
                                "-m" "isort" "-" args))
                 (tick (buffer-chars-modified-tick)))
-            (cond
-             ((eq 1 status)
+            (unless (eq 0 status)
               (error "%s exited with status %s (maybe isort is missing?)"
                      python-interpreter status))
-             ((eq 2 status)
-              (error "%s exited with status %s (maybe isort version is <5.7.0?)"
-                     python-interpreter status)))
             (replace-buffer-contents temp)
             (not (eq tick (buffer-chars-modified-tick)))))))))
 
-- 
2.39.2

bug#64406: [PATCH] Improve commands to manage Python imports

[Eli Zaretskii]

> From: Matthias Meulien <orontee@gmail.com>
> Cc: arstoffel@gmail.com,  64406@debbugs.gnu.org
> Date: Thu, 13 Jul 2023 23:07:40 +0200
> 
> > Thanks.  Could you please make a single patch from these two?  There's
> > no need to revert the original one in a separate patch.
> 
> Yes.
> 
> >> Subject: [PATCH 1/2] Fix "Improve Python imports management commands"
> >> 
> >> * lisp/progmodes/python.el (python--do-isort): The isort library don't
> >> check for its version itself!
> >
> > I don't think I understand this log entry.  What does it mean "the
> > isort library don't check its version"?
> 
> You're right, it doesn't make sense.
> 
> 
> >> +             ((eq 3 status) " (maybe isort version is less than 5.7.0?)")
> >                                                       ^^^^^^^
> > Here, "is older" should be more clear, I think.
> 
> Ok.

Thanks, installed on master.

Please in the future always mention the bug number, when known, in the
commit log message (I did it this time for you).