Set the working dir and shadow file flag for mypy #9

dmitrig · 2022-10-28T19:54:47Z

Mypy requires some configuration in most cases, which it looks for in its working directory by default. You can check that flymake-collection-mypy won't work with projects like urllib3 at the moment. This updates flymake-collection-mypy to set default-directory to a parent dir containing a config file, if it exists. It also adds the mypy --shadow-file flag to make sure types are defined in the right module and caching works when processing temp files. Also includes minor tweaks to make flags customizable and parse out the diagnostic id.

Some things to consider:

Making setting the cwd opt-in, if it's important to preserve the current behavior
Adding a separate :working-dir keyword arg like flycheck instead of using :pre-let
I had to remove the leading dot from temp files, which appears to break relative imports. This will affect all checkers, but it could be made configurable per-checker

mohkale

Hi, thanks for this. Just a few things I think should be changed.

mohkale · 2022-10-30T09:16:11Z

src/flymake-collection-define.el

-       (make-temp-file ".flymake_" nil (concat "_" basename))))))
+       (make-temp-file "flymake_" nil (concat "_" basename))))))


What is the significance of this change?

See:

I had to remove the leading dot from temp files, which appears to break relative imports. This will affect all checkers, but it could be made configurable per-checker

Python's module system can't handle file names with leading dots, so this causes some spurious errors when running mypy on temp files. I can provide an example, if you're interested. The --shadow-file flag avoids the issue but only works when the source file exists, so checking unsaved buffers would still be broken without this change.

Bit of an edge case, but making these short-lived temp files hidden didn't seem particularly important, either. Happy to revert or consider other options.

Hmmm... the motivation is understandable but the end the result isn't ideal IMO 😞. Dot prefixed files are a standard for hidden files on UNIX (and more recently windows since it stopped restricting their creation). Most systems (example file browsers) will auto hide them. Admittedly that isn't a big concern for something so short lived but I'd still like to retain the intent that these are hidden files, especially when their in a project directory instead of /tmp/.

Could you instead make this a generic parameter that can be set by the linter itself and keep the default as .. Should be just a new parameter to the define macro here (and the helpers below as well).

Added a temp-file-prefix parameter with a default. After working with flymake-collection-define a bit, I'd have to agree with the user in #2 re: reducing the use of macros. Seems like a lot of the logic could be moved into functions in flymake-collection.el, which all the checkers already require.

mohkale · 2022-10-30T09:22:54Z

src/checkers/flymake-collection-mypy.el

+  :type 'list
+  :group 'flymake-collection)
+
+(defun flymake-collection-mypy--default-directory (buffer)


Please add an option to enable or disable this functionality and use the project library to find the project root instead of locate-dominating-file.

Made it a bit more general: it can now try any of those options in order. Default is to look for config files, then try the project root, then fall back to the buffer default-directory. You can also use a directory path.

mohkale · 2022-10-30T09:23:16Z

src/checkers/flymake-collection-mypy.el

-   (warning bol (file-name) ":" line ":" column ": warning: " (message) eol)
-   (note    bol (file-name) ":" line ":" column ": note: "    (message) eol)))
+  ((error   bol (file-name) ":" line ":" column ": error: "
+            (message) "  [" (id (one-or-more not-newline)) "]" eol)


Some new tests ensuring id is parsed correctly would be great.

This appears to break relative imports in files checked by mypy. Maybe make the temp file pattern configurable per checker?

Fall back to the buffer name when `buffer-file-name' is nil. Necessary for checking temp files since python module names are based on the file name.

fbergroth · 2022-11-11T14:20:56Z

src/checkers/flymake-collection-mypy.el

 (require 'flymake)
 (require 'flymake-collection)

 (eval-when-compile
  (require 'flymake-collection-define))

+(defcustom flymake-collection-mypy-args
+  '("--show-column-numbers"


Perhaps --show-column-numbers and other args that the regexps depend upon shouldn't be customizable?

jgarte · 2022-11-27T19:34:42Z

Hi, what's the status on this? This PR would be so awesome to get merged in for me. I'll be using it.

mohkale

Hi, sorry for the late update. I'd been meaning to come back to this.

mohkale · 2022-11-27T20:18:37Z

src/flymake-collection-define.el

-       (make-temp-file ".flymake_" nil (concat "_" basename))))))
+       (make-temp-file "flymake_" nil (concat "_" basename))))))


Hmmm... the motivation is understandable but the end the result isn't ideal IMO 😞. Dot prefixed files are a standard for hidden files on UNIX (and more recently windows since it stopped restricting their creation). Most systems (example file browsers) will auto hide them. Admittedly that isn't a big concern for something so short lived but I'd still like to retain the intent that these are hidden files, especially when their in a project directory instead of /tmp/.

Could you instead make this a generic parameter that can be set by the linter itself and keep the default as .. Should be just a new parameter to the define macro here (and the helpers below as well).

mohkale · 2022-11-27T20:25:04Z

src/checkers/flymake-collection-mypy.el

+        ((res (cond
+               ((eq spec 'default-directory)
+                (buffer-local-value 'default-directory buffer))
+               ((eq spec 'project-root)


Thanks for adding a project root check. I'm still not sure whether a recursive search up through the directory tree is the best (or at the very least a good default) choice here. Especially if this is something that's being edited across TRAMP or something else which adds a level of latency. That said I also can't think of a very good alternative either, this should really be something the tools themselves can pick up on IMO, not the packages that invoke those rules. That said they don't, so we should 🙃. I'm happy to approve this section. At some point it'd be nice if we could refactor it into something general purpose enough it could be used with the other python linters (they all have a habit of depending on this).

Pretty sure sure this is how flycheck does it. I agree it'd be nice to, at the very least, avoid searching directories every time the checker is invoked. Actually, at first I only had this run only once per buffer, but initializing checkers and caching some state seemed like something that should also be done uniformly, rather than in an ad-hoc way per checker.

At some point it'd be nice if we could refactor it into something general purpose...

Don't think there's anything mypy or Python specific in either flymake-collection-mypy--locate-dominating-files or flymake-collection-mypy--default-directory if you want to make those available to other checkers.

src/checkers/flymake-collection-mypy.el

mohkale · 2022-12-19T18:54:44Z

LGTM. Thanks @dmitrig.

mohkale requested changes Oct 30, 2022

View reviewed changes

dmitrig added 7 commits November 3, 2022 00:52

Run mypy from inferred python project root

1a52a9a

Remove leading dot from temp file name

a30c31b

This appears to break relative imports in files checked by mypy. Maybe make the temp file pattern configurable per checker?

Use mypy --shadow-file for buffers with a file

06feaa5

Fall back to the buffer name when `buffer-file-name' is nil. Necessary for checking temp files since python module names are based on the file name.

Parse mypy diagnostic ids

a0c6f88

Make the mypy working dir configurable

4b69516

Add basic tests for mypy

9762880

Add require for project

75b867a

dmitrig force-pushed the mypy-project-root branch from 23f9caf to 75b867a Compare November 3, 2022 04:55

dmitrig requested a review from mohkale November 4, 2022 16:45

mohkale mentioned this pull request Nov 10, 2022

Extend mypy checker command with additional customizable arguments #13

Closed

fbergroth reviewed Nov 11, 2022

View reviewed changes

dmitrig added 2 commits November 12, 2022 11:24

Move required mypy flags out of custom variable

6ef91a4

Fix parsing of mypy error ids with hyphens

c998cba

mohkale reviewed Nov 27, 2022

View reviewed changes

Add temp-file-prefix paramter

9294c56

dmitrig requested a review from mohkale December 14, 2022 18:31

mohkale reviewed Dec 19, 2022

View reviewed changes

src/checkers/flymake-collection-mypy.el Show resolved Hide resolved

Update src/checkers/flymake-collection-mypy.el

87a3018

mohkale approved these changes Dec 19, 2022

View reviewed changes

mohkale merged commit 74e2f3f into mohkale:release Dec 19, 2022

This was referenced Dec 20, 2022

Package for GNU Guix #8

Closed

configure mypy CLI flags? #6

Closed

Propose your package to NonGNU ELPA or GNU ELPA? #2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set the working dir and shadow file flag for mypy #9

Set the working dir and shadow file flag for mypy #9

dmitrig commented Oct 28, 2022

mohkale left a comment

mohkale Oct 30, 2022

dmitrig Nov 1, 2022

mohkale Nov 27, 2022

dmitrig Dec 13, 2022 •

edited

Loading

mohkale Oct 30, 2022

dmitrig Nov 2, 2022

mohkale Oct 30, 2022

fbergroth Nov 11, 2022

jgarte commented Nov 27, 2022 •

edited

Loading

mohkale left a comment

mohkale Nov 27, 2022

mohkale Nov 27, 2022

dmitrig Dec 13, 2022 •

edited

Loading

mohkale commented Dec 19, 2022

		(make-temp-file ".flymake_" nil (concat "_" basename))))))
		(make-temp-file "flymake_" nil (concat "_" basename))))))

Set the working dir and shadow file flag for mypy #9

Set the working dir and shadow file flag for mypy #9

Conversation

dmitrig commented Oct 28, 2022

mohkale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrig Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgarte commented Nov 27, 2022 • edited Loading

mohkale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrig Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

mohkale commented Dec 19, 2022

dmitrig Dec 13, 2022 •

edited

Loading

jgarte commented Nov 27, 2022 •

edited

Loading

dmitrig Dec 13, 2022 •

edited

Loading