New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use python -c to invoke pylint and flake8 #1113

Merged
merged 2 commits into from Feb 17, 2018

Conversation

Projects
None yet
5 participants
@cpitclaudel
Member

cpitclaudel commented Sep 27, 2016

Use python -m to invoke pylint and flake8
Part of GH-1055.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Sep 28, 2016

LGTM :)

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 28, 2016

@cpitclaudel I must admit that I'm not sure whether I'd like to merge this pull request. Can we discuss the problems that this is supposed to solve first, over at #1055?

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 28, 2016

@cpitclaudel Independently of the discussion in #1055 we need custom :predicates for these syntax checkers now since we need to check whether the modules are actually available.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Sep 28, 2016

@cpitclaudel Independently of the discussion in #1055 we need custom :predicates for these syntax checkers now since we need to check whether the modules are actually available.

Ah, very good point. That is one downside of this PR. :predicate or :enabled?

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 28, 2016

@cpitclaudel I'd go for :enabled. After all that's not something that'd change frequently would it?

Generally I think we should slowly move to use :enabled for anything that checks whether a checker is installed, as these are facts that scarcely change. We should leave :predicate for checks that really change frequently, like whether the buffer is saved, or whether it contains a certain text or so.

@lunaryorn

Misses an :enabled that checks whether the corresponding modules are installed.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 28, 2016

LGTM, but I "requested changes" for the missing :enabled.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Oct 27, 2016

@cpitclaudel Did you get a chance to work on this pull request meanwhile?

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Oct 27, 2016

Barely. I'll try to get some time before the week-end

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Oct 27, 2016

@cpitclaudel No hurry, was just curious.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jul 17, 2017

@cpitclaudel Any update on this one?

@cpitclaudel cpitclaudel force-pushed the 1055-python-executable branch from 4f4b375 to 9185eb3 Jan 28, 2018

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Jan 28, 2018

I looked into this again, but there's a problem I didn't consider at the time: backwards compatibility. If people have custom paths for flake8 or pylint, this change will break them.

There's multiple ways to proceed.

  • Accept the breakage, document it, and move on; the problem is that the fix for users won't be entirely trivial (it's tricky to load a python module that's not in your Python path), unless they use a venv in which flake8 or pylint is installed (in which case they'll just have to point Flycheck to the venv's Python instead of the venv's pylint.

  • Make it possible to use either form: we add a new variable flycheck-flake8-python-executable, which if set takes precedence over flycheck-flake8-executable. This might be tricky to implement in today's Flycheck.

  • Same as above, but use a single variable for the executable, and add -m flake8 if we think the executable is python instead of a flake8 binary. We could either do the detection automatically (pattern-match for python(\d(\.\d)?)?(\.exe)?) or add a setting. If we do that, we can also change the default value of the checker executable to python, so we keep backwards compatibility while getting the benefits of python -m

  • Abandon this PR. I don't like that much, because I think it's harder than it should be ATM to mix python2 and python3 sources in Flycheck.

WDYT?

@dakra

This comment has been minimized.

dakra commented Jan 29, 2018

Is it possible to make python -m flake8 the default command and if someone customizes it, it'll just use that?

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Jan 29, 2018

Is it possible to make python -m flake8 the default command and if someone customizes it, it'll just use that?

Not easily (at least I think, but I'd like to be proven wrong); Flycheck has a pretty deeply-embedded notion of checker executables, where each checker has it's own binary + arguments passed to it. python -m flake8 is an executable + 2 arguments, so what I'm proposing to do is to change the flake8 executable to "python" instead of flake8, and to extend the argument list with -m flake8; for compatibility, I'm also suggesting to make the argument list additions conditional, so that they only happen if the executable "looks like" python, rather than flake8.

@ibizaman

This comment has been minimized.

ibizaman commented Feb 1, 2018

Not especially pushing for this PR, but one use-case I have where calling pylint through python -m pylint is necessary is when having a .pylintrc like so:

[MASTER]

load-plugins=
    my_module.my_helper

This is the error I get in emacs:

Suspicious state from syntax checker python-pylint: Flycheck checker python-pylint returned non-zero exit code 1, but its output contained no errors: Using config file ./.pylintrc
Traceback (most recent call last):
  File ".virtualenvs/python2.7/bin/pylint", line 11, in <module>
    sys.exit(run_pylint())
  File ".virtualenvs/python2.7/lib/python2.7/site-packages/pylint/__init__.py", line 16, in run_pylint
    Run(sys.argv[1:])
  File ".virtualenvs/python2.7/lib/python2.7/site-packages/pylint/lint.py", line 1312, in __init__
    linter.load_plugin_modules(plugins)
  File ".virtualenvs/python2.7/lib/python2.7/site-packages/pylint/lint.py", line 494, in load_plugin_modules
    module = modutils.load_module_from_name(modname)
  File ".virtualenvs/python2.7/lib/python2.7/site-packages/astroid/modutils.py", line 190, in load_module_from_name
    return load_module_from_modpath(dotted_name.split('.'), path, use_sys)
  File ".virtualenvs/python2.7/lib/python2.7/site-packages/astroid/modutils.py", line 232, in load_module_from_modpath
    mp_file, mp_filename, mp_desc = imp.find_module(part, path)
ImportError: No module named my_module

Try installing a more recent version of python-pylint, and please open a bug report if the issue persists in the latest release.  Thanks!

I fixed that in another way, by adding:

  (with-eval-after-load 'flycheck
    (setq flycheck-command-wrapper-function
          (lambda (command)
            (if (null (string-match "pylint" (car command)))
                command
                (let* ((new-prefix (replace-regexp-in-string "pylint$" "python" (car command)))
                       (new-rest (append '("-m" "pylint") (cdr command)))
                       (new-command (append (list new-prefix) new-rest)))
                  new-command)))))

I also needed to set the default-directory to be the project root instead of the file's directory because other my_module wasn't always found. I did that in the .dir-locals.el with:

((nil . ((eval . (setq default-directory (locate-dominating-file default-directory ".dir-locals.el"))))))

EDIT: corrected the flycheck-command-wrapper-function snippet as it was a bit too aggressive.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Feb 1, 2018

Thank, that's an interesting data point. I think your solution is similar to mine.
How does your working directory setup looks like?

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Feb 10, 2018

I looked into this again, but there's a problem I didn't consider at the time: backwards compatibility. If people have custom paths for flake8 or pylint, this change will break them.

I'm not sure it's worth hanging onto backward compatibility on this one. python-pycompile is already using python -m. If users want to preserve the previous behavior, they can always override the checker with the previous version.

In this case, there is an even simpler trick:

(setf (flycheck-checker-get 'python-flake8 'command)
      (cddr (flycheck-checker-get 'python-flake8 'command))

would drop the python -m part of the command and restore the previous behavior.

flycheck.el Outdated
@@ -8022,6 +8040,10 @@ Requires Flake8 3.0 or newer. See URL
(id (one-or-more (any alpha)) (one-or-more digit)) " "
(message (one-or-more not-newline))
line-end))
:enabled (lambda () (and flycheck-flake8-python-executable

This comment has been minimized.

@fmdkdd

fmdkdd Feb 10, 2018

Member

Is this a new variable that's not in this branch? Or did you mean flycheck-python-flake8-executable? The latter is already checked by Flycheck.

@cpitclaudel cpitclaudel force-pushed the 1055-python-executable branch from 9185eb3 to 3971a82 Feb 12, 2018

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Feb 12, 2018

I updated the PR, but there's one more issue. From the docs of python -m:

If this option is given, the first element of sys.argv will be the full path to the module file. As with the -c option, the current directory will be added to the start of sys.path.

The addition of the current directory means that large projects with names that conflict with the standard library, like Sphinx, can't be checked:

python -m pylint -r n --output-format text --msg-template \{path\}\:\{line\}\:\{column\}\:\{C\}\:\{symbol\}\:\{msg\} --rcfile /home/clement/.emacs.d/external-config/.pylintrc /home/clement/.local/lib/python3.5/site-packages/sphinx/environment.py
Traceback (most recent call last):
  File "/usr/lib/python2.7/runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/home/clement/.local/lib/python2.7/site-packages/pylint/__main__.py", line 10, in <module>
    pylint.run_pylint()
  File "/home/clement/.local/lib/python2.7/site-packages/pylint/__init__.py", line 16, in run_pylint
    from pylint.lint import Run
  File "/home/clement/.local/lib/python2.7/site-packages/pylint/lint.py", line 57, in <module>
    import astroid
  File "/home/clement/.local/lib/python2.7/site-packages/astroid/__init__.py", line 54, in <module>
    from astroid.nodes import *
  File "/home/clement/.local/lib/python2.7/site-packages/astroid/nodes.py", line 54, in <module>
    from astroid.scoped_nodes import (
  File "/home/clement/.local/lib/python2.7/site-packages/astroid/scoped_nodes.py", line 25, in <module>
    import io
  File "io.py", line 12, in <module>
    from docutils.readers import standalone
  File "/usr/local/lib/python2.7/dist-packages/docutils/readers/standalone.py", line 13, in <module>
    from docutils import frontend, readers
  File "/usr/local/lib/python2.7/dist-packages/docutils/frontend.py", line 38, in <module>
    import optparse
  File "/usr/lib/python2.7/optparse.py", line 419, in <module>
    _builtin_cvt = { "int" : (_parse_int, _("integer")),
  File "/usr/lib/python2.7/gettext.py", line 584, in gettext
    return dgettext(_current_domain, message)
  File "/usr/lib/python2.7/gettext.py", line 548, in dgettext
    codeset=_localecodesets.get(domain))
  File "/usr/lib/python2.7/gettext.py", line 483, in translation
    mofiles = find(domain, localedir, languages, all=1)
  File "/usr/lib/python2.7/gettext.py", line 440, in find
    for nelang in _expand_lang(lang):
  File "/usr/lib/python2.7/gettext.py", line 133, in _expand_lang
    from locale import normalize
ImportError: cannot import name normalize

(this happens because Sphinx contains a "locale" module.

@cpitclaudel cpitclaudel force-pushed the 1055-python-executable branch from 3971a82 to 7c8ce34 Feb 12, 2018

@cpitclaudel cpitclaudel changed the title from Use python -m to invoke pylint and flake8 to Use python -c to invoke pylint and flake8 Feb 12, 2018

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Feb 12, 2018

Fixed by moving to python -c. Let me know what you think.

@cpitclaudel cpitclaudel force-pushed the 1055-python-executable branch from 7c8ce34 to 71b2c7a Feb 12, 2018

@cpitclaudel cpitclaudel force-pushed the 1055-python-executable branch 7 times, most recently from 2181b02 to 8154437 Feb 12, 2018

@fmdkdd

fmdkdd approved these changes Feb 16, 2018

LGTM. I'm not convinced of the need for backward-compatibility on this one, but I don't write enough Python, so I'll trust your judgment.

This can go into CHANGES.rst Improvements section.

@@ -8764,7 +8803,8 @@ Update the error level of ERR according to
Requires Flake8 3.0 or newer. See URL
`https://flake8.readthedocs.io/'."
:command ("flake8"

This comment has been minimized.

@fmdkdd

fmdkdd Feb 16, 2018

Member

Please add a comment saying why we don't call flake8 directly and linking to this PR (or #1055).

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 17, 2018

Member

Good idea

flycheck.el Outdated
"Determines whether CHECKER needs a `-m module-name' argument.
Previous versions of Flycheck called pylint and flake8 directly;
this check ensures that we don't break existing code."
(not (string-match-p (concat "\\(pylint\\|flake8\\)"

This comment has been minimized.

@fmdkdd

fmdkdd Feb 16, 2018

Member

Could you use rx to make the regexp more readable?

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 17, 2018

Member

Sure. Sounds like the perfect time to advertise https://github.com/cpitclaudel/easy-escape, too :)

flycheck.el Outdated
(string-trim (buffer-string))))))
(defun flycheck-python-needs-module-p (checker)
"Determines whether CHECKER needs a `-m module-name' argument.

This comment has been minimized.

@fmdkdd

fmdkdd Feb 16, 2018

Member

I guess it's not really -m module-name, since we end up using -c instead. But the intent is the same?

This comment has been minimized.

@cpitclaudel

cpitclaudel Feb 17, 2018

Member

Thanks, I changed the docstring.

Use python -c to invoke pylint and flake8
Part of GH-1055.  Ideally we'd use 'python -m ...' instead of 'python -c ...',
but Python adds the current directory to sys.path in -c and -m mode, which
confuses pylint (for example, 'python -m pylint' breaks when Flychecking Sphinx,
whose sources include a 'locale' directory that conflicts with Python's standard
'locale' module).

@cpitclaudel cpitclaudel force-pushed the 1055-python-executable branch from 8154437 to 6e45989 Feb 17, 2018

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Feb 17, 2018

This can go into CHANGES.rst Improvements section.

Yup, I already added it :)

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Feb 17, 2018

Question: do we actually care about argument order in docstrings? I'm temper to turn checkdoc-arguments-in-order-flag off.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Feb 17, 2018

I don't mind about the order no.

@cpitclaudel cpitclaudel force-pushed the 1055-python-executable branch from 61fbf04 to 1100f1d Feb 17, 2018

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Feb 17, 2018

OK, all green.

@fmdkdd fmdkdd merged commit 1ec9397 into master Feb 17, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@fmdkdd fmdkdd deleted the 1055-python-executable branch Feb 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment