bug#28821: [PATCH] Implement Python backend for Flymake

[joaotavora@gmail.com (João Távora)]

Hi Lele,

Here are some comments:

Lele Gaifax <lele@metapensiero.it> writes:

> +(defgroup python-flymake nil
> +  "Integration between Python and Flymake."
> +  :group 'python
> +  :link '(custom-group-link :tag "Flymake" flymake)
> +  :version "26.1")
> +
> +(defcustom python-flymake-command '("pyflakes")
> +  "The external tool that will be used to perform the syntax check.
> +This is a non empty list of strings, the checker tool possibly followed by
> +required arguments: to use `flake8' you would set this to (\"flake8\" \"-\")."
>
I wonder if you shouldn't mention here that the command produced should,
once invoked, check (a file? a chunk?) of python source code passed to
it via its standard input.

> +  :group 'python-flymake
> +  :type '(repeat string))
> +
> +;; The default regexp accomodates for older pyflakes, which did not
> +;; report the column number
> +(defcustom python-flymake-command-output-regexp
> +  "^\\(?:<stdin>\\):\\(?1:[0-9]+\\):\\(?:\\(?2:[0-9]+\\):\\)? \\(?3:.*\\)$"
> +  "The regexp used to parse the output of the specified tool.
> +It must contain two or three groups: group 1 is the line number, group 2 the
> +optional column number and the third is the actual message."

A common trick here that old flymake (and also compile.el) use is to
define the variable's value as list (REGEXP LINE COLUMN TYPE
MESSAGE). REGEXP is mandatory. LINE, COLUMN, TYPE and MESSAGE are
non-negative integer numbers designating regexp groups, or nil. In the
latter case it means the regexp cannot capture that entity.

So in your case it would become

(defcustom python-flymake-command-output-regexp
  (list
    "^\\(?:<stdin>\\):\\(?1:[0-9]+\\):\\(?:\\(?2:[0-9]+\\):\\)? \\(?3:.*\\)$"
    1 2 nil 3)
  "docstring" :group 'python-flymake
  :type '(list string (choice integer symbol)
                      (choice integer symbol)
                      (choice integer symbol)
                      (choice integer symbol)))

Perhaps TYPE does not make much sense currently. But it would match
slightly better with compilation-error-regexp-alist in the future (which
you should see).

> +  (unless (derived-mode-p 'python-mode)
> +    (error "Can only work on `python-mode' buffers"))

Stefan and I arrived at the conclusion that this is cruft and isn't
needed. 

> +(defun python-flymake-activate ()

Rename this to python--flymake-setup, because "activation" is actually
enabling flymake-mode. Also, I think you should add an autoload cookie
to python--flymake-setup and then call that function from the end of
python-mode. The

> +  "Activate the Flymake syntax check on all python-mode buffers."
> +  (add-hook 'flymake-diagnostic-functions #'python-flymake nil t))

I'd use 'python-flymake instead of #'python-flymake in add-hook, but I
can't offer a sound reason why :-)