Repository: spyder-ide/spyder
Pull request: PR: Make pylint plugin find most local .pylintrc to given file and set cwd correctly
<!--- Make sure to read the Contributing Guidelines:                   --->
<!--- https://github.com/spyder-ide/spyder/blob/master/CONTRIBUTING.md --->
<!--- and follow PEP 8, PEP 257 and Spyder's code style:               --->
<!--- https://github.com/spyder-ide/spyder/wiki/Dev:-Coding-Style      --->

## Description of Changes


* [x] Wrote at least one-line docstrings (for any new functions)
* [x] Added unit test(s) covering the changes (if testable)
<!--- Remember that an image/animation is worth a thousand words! --->
* [x] ~~Included a screenshot or animation (if affecting the UI, see [Licecap](https://www.cockos.com/licecap/))~~


<!--- Explain what you've done and why --->

Supersedes PR #10751 by @tovogt . This should be ready to merge as-is, presuming tests pass on the CIs (as they all did locally).

Previously, the Pylint plugin (Static Code Analysis pane) would not find the correct .pylintrc configuration file (or any config file at all) when run, either in the working dir, the project dir or the home directory. Furthermore, the working directory was not set to match that of Spyder. With these improvements, which were originally implemented on my suggestion by @tovogt , the cwd is now set correctly, and more importantly the most proximate `.pylintrc` file is retrieved and used, smoothly integrated with Pylint's built-in search path: 

1. `.pylintrc` (or `pylintrc`, `pyproject.toml`, or `setup.cfg`) or in the file's directory*
2. Any `.pylintrc` (etc.) in the Python module hierarchy at and above the file, at the closest level to the file*
3. `.pylintrc` (etc.) in Spyder's working directory*
4. `.pylintrc` (etc) in Spyder's active project directory (if any)*
5. `.pylintrc` (etc) named in the `PYLINTRC` environment variable
6. `.pylintrc` (etc) or `.config/pylintrc` in your home directory
7. `/etc/pylintrc` (on POSIX, obviously)

* Denotes items added/modified on the search path relative to pylint's naive config

In addition, I've implemented a battery of comprehensive unit and integration tests for this functionality.

### Issue(s) Resolved

<!--- List the issue(s) below, in the form "Fixes #1234"; one per line --->

Fixes #5345


### Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the [Developer Certificate of Origin](https://developercertificate.org)
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

<!--- TYPE YOUR USER/NAME AFTER THE FOLLOWING: --->
I certify the above statement is true and correct: CAM-Gerlach

<!--- Thanks for your help making Spyder better for everyone! --->


Problem statement:
Static Code Analysis Not Reading .pylintrc file
I have a .pylintrc file in the Spyder project directory and in the file directory (it is located in a subdirectory). It has the following line in the [MASTER] block:

extension-pkg-whitelist=numpy

It also has the following lines in the [TYPECHECK] block:
ignored-modules=numpy
ignored-classes=optparse.Values,thread._local,_thread._local,numpy

However, the code analysis still gives me an error on the second line of these two:
time1 = df.index.to_datetime()
time2 = np.array([time1.year, time1.month, time1.day,
                          time1.hour, time1.minute,
                          time1.second, time1.microsecond/1000]).transpose()

The error is [E1101] Instance of 'list' has no 'transpose' member

Is there another directory that Spyder's call of Pylint must be reading from? How do I check this? Also, my project is on a shared network drive (temporarily). 

## Versions and main components

* Spyder Version: 3.2.3
* Python Version: Anaconda 3.6
* Qt Version: 5.6.2
* PyQt Version: 5.6.0
* Operating system: Windows 7

Hints from issue discussion:
Sorry, at the moment we don't read the contents `.pylintrc` when running Pylint inside Spyder.

You're welcome to send us a pull request to provide this functionality.
I've been able to do this just fine on a per-user basis on Windows by just editing the .pylintrc file in my user folder (`/Users/username/,pylintrc`). I would guess what is missing is per-project reading/overriding of this?
I think so, yeah.
sorry to necro-bump this; but is this issue (really) still open?
just ran into the same issue with the wxpython package. In VS Code, I can circumvent this by adding extension-pkg-whitelist=wx to the settings.json. But most of the time I prefer to use Spyder and [CAMGerlach-Baron](https://github.com/CAMGerlach-Baron)'s solution unfortunately does not work for me; pylint seems to ignore the pylintrc file I tell it to use...
I suggest using `self.process.setWorkingDirectory(getcwd_or_home())` in https://github.com/spyder-ide/spyder/blob/master/spyder/plugins/pylint/widgets/pylintgui.py#L320 and specifying the full path to the file in https://github.com/spyder-ide/spyder/blob/master/spyder/plugins/pylint/widgets/pylintgui.py#L341 and https://github.com/spyder-ide/spyder/blob/master/spyder/plugins/pylint/widgets/pylintgui.py#L343

Alternatively, we could check for a `.pylintrc` file in the project directory and - if found - make note of it in the GUI widget. In the widget we can give the user an option to use this `.pylintrc` (checkbutton) and - if so - explicitly pass the path to the `.pylintrc` file by adding the `--rcfile` option to the `p_args` in https://github.com/spyder-ide/spyder/blob/master/spyder/plugins/pylint/widgets/pylintgui.py#L348

The first option is easier to implement and doesn't bother the user with any new information or choices. However, the user might be surprised if a file is silently processed with the project's `.pylintrc` - especially because this was not the case until today. The second option adds more clutter, but makes the behavior more transparent and gives the user a choice. Furthermore, adding the `--rcfile` option while using the file's basedir as working directory will prevent pylint from using the other ways of specifying projectwide pylint configuration (e.g. pyproject.toml, see https://pylint.readthedocs.io/en/latest/user_guide/run.html#command-line-options).

A third option would be the first one with a checkbutton "Run pylint from the current project's root directory" in the widget that allows to switch to the current behavior.

What do you think? Which option do you prefer?
@tovogt, thanks for your suggestions. We were thinking to implement something similar with @goanpeca.

@goanpeca, what do you think?
Yes, indeed this is in out to do list. I prefer the option that does not add any extra options for the user and does "the right thing"

Is there an actual use case where a user might be with a project open and wants to use a different pylint? like the global one? if this is the case then indeed we might have to add an option to select between global pylint config and current project config (if a project is open of course)
A user could have a `.pylintrc` in the project's root and additional `.pylintrc` files in subdirectories that are supposed to apply only to the files in the subdirectories. This is an open issue with pylint, see https://github.com/PyCQA/pylint/issues/618.

You could even interpret the current issue as a pylint bug since a user might expect pylint to navigate recursively to parent directories and to apply all `.pylintrc` settings found there. If this was the case, we could just take the file's dirname as working directory (current behavior of this plugin).
Hi, can I ask if this is still true? 
"Sorry, at the moment we don't read the contents .pylintrc when running Pylint inside Spyder."
I have tried putting .pylintrc in various places, even in the environmental variable PYLINTRC , but it seems like running Code Analysis within Spyder does not read any .pylintrc file at the moment. It would be good if you could confirm, thanks.
> Hi, can I ask if this is still true?
> "Sorry, at the moment we don't read the contents .pylintrc when running Pylint inside Spyder."

Yes, it is. We'll try to fix it in a couple of months.

Commit message: Skip new pylint test on PY2 and other tweaks for reliable CIs

Patch to review:
diff --git a/spyder/app/tests/test_mainwindow.py b/spyder/app/tests/test_mainwindow.py
index ffabecd7f5f..9e7618395f8 100644
--- a/spyder/app/tests/test_mainwindow.py
+++ b/spyder/app/tests/test_mainwindow.py
@@ -2996,8 +2996,9 @@ def test_runcell_after_restart(main_window, qtbot):
     # Make sure no errors are shown
     assert "error" not in shell._control.toPlainText().lower()
 
+
 @pytest.mark.slow
-@flaky(max_runs=1)
+@flaky(max_runs=3)
 @pytest.mark.parametrize(
     "ipython", [True, False])
 @pytest.mark.parametrize(
diff --git a/spyder/plugins/pylint/tests/test_pylint.py b/spyder/plugins/pylint/tests/test_pylint.py
new file mode 100644
index 00000000000..19cddbbb39a
--- /dev/null
+++ b/spyder/plugins/pylint/tests/test_pylint.py
@@ -0,0 +1,169 @@
+# -*- coding: utf-8 -*-
+# ----------------------------------------------------------------------------
+# Copyright © Spyder Project Contributors
+#
+# Licensed under the terms of the MIT License
+# ----------------------------------------------------------------------------
+"""Tests for the execution of pylint."""
+
+from __future__ import unicode_literals
+
+# Standard library imports
+from io import open
+import os.path as osp
+
+# Third party imports
+import pytest
+
+# Local imports
+from spyder.plugins.pylint.widgets.pylintgui import (
+    get_pylintrc_path,
+    PylintWidget,
+    )
+from spyder.py3compat import PY2
+
+# pylint: disable=redefined-outer-name
+
+PYLINTRC_FILENAME = ".pylintrc"
+
+# Constants for dir name keys
+# TODO in Python 3 : Replace with enum
+NO_DIR = "e"
+SCRIPT_DIR = "SCRIPT_DIR"
+WORKING_DIR = "WORKING_DIR"
+PROJECT_DIR = "PROJECT_DIR"
+HOME_DIR = "HOME_DIR"
+ALL_DIR = "ALL_DIR"
+
+DIR_LIST = [SCRIPT_DIR, WORKING_DIR, PROJECT_DIR, HOME_DIR]
+DIR_LIST_ALL = [NO_DIR] + DIR_LIST + [ALL_DIR]
+
+PYLINT_TEST_SCRIPT = "\n".join(
+    [dir_name + " = " + str(idx) for idx, dir_name in enumerate(DIR_LIST_ALL)])
+PYLINT_TEST_SCRIPT = "\"\"\"Docstring.\"\"\"\n" + PYLINT_TEST_SCRIPT + "\n"
+
+PYLINTRC_TEST_CONTENTS = """
+[MESSAGES CONTROL]
+enable=blacklisted-name
+
+[BASIC]
+bad-names={bad_names}
+good-names=e
+"""
+
+
+@pytest.fixture
+def pylintrc_search_paths(tmp_path_factory):
+    """Construct temporary .pylintrc search paths."""
+    search_paths = {dir_name: str(tmp_path_factory.mktemp(dir_name))
+                    for dir_name in DIR_LIST}
+    return search_paths
+
+
+@pytest.fixture
+def pylint_test_script(pylintrc_search_paths):
+    """Write a script for testing Pylint to a temporary directory."""
+    script_path = osp.join(
+        pylintrc_search_paths[SCRIPT_DIR], "test_script.py")
+    with open(script_path, mode="w",
+              encoding="utf-8", newline="\n") as script_file:
+        script_file.write(PYLINT_TEST_SCRIPT)
+    return script_path
+
+
+@pytest.fixture(
+    params=[
+        [], [SCRIPT_DIR], [WORKING_DIR], [PROJECT_DIR], [HOME_DIR],
+        [SCRIPT_DIR, HOME_DIR], [WORKING_DIR, PROJECT_DIR],
+        [SCRIPT_DIR, PROJECT_DIR], [PROJECT_DIR, HOME_DIR],
+        [SCRIPT_DIR, WORKING_DIR, PROJECT_DIR, HOME_DIR]],
+    ids=["None", "Script", "Working", "Project", "Home", "Script & Home",
+         "Working & Project", "Script & Working", "Project & Home", "All"])
+def pylintrc_files(pylintrc_search_paths, request):
+    """Store test .pylintrc files at the paths and determine the result."""
+    search_paths = pylintrc_search_paths
+
+    # Determine the bad names that should be reported
+    pylintrc_locations = request.param
+    bad_names = [ALL_DIR]
+    for search_path_name, search_path in search_paths.items():
+        if search_path_name in pylintrc_locations:
+            expected_path = osp.join(search_path, PYLINTRC_FILENAME)
+            bad_names += [search_path_name]
+            break
+    else:
+        expected_path = None
+        bad_names = [NO_DIR]
+
+    # Store the selected pylintrc files at the designated paths
+    for location in pylintrc_locations:
+        pylintrc_test_contents = PYLINTRC_TEST_CONTENTS.format(
+            bad_names=", ".join([location, ALL_DIR]))
+        pylintrc_path = osp.join(search_paths[location], PYLINTRC_FILENAME)
+        with open(pylintrc_path, mode="w",
+                  encoding="utf-8", newline="\n") as rc_file:
+            rc_file.write(pylintrc_test_contents)
+    return search_paths, expected_path, bad_names
+
+
+def test_get_pylintrc_path(pylintrc_files, mocker):
+    """Test that get_pylintrc_path finds the expected one in the hiearchy."""
+    search_paths, expected_path, __ = pylintrc_files
+    mocker.patch("pylint.config.os.path.expanduser",
+                 return_value=search_paths[HOME_DIR])
+    actual_path = get_pylintrc_path(
+        search_paths=list(search_paths.values()),
+        home_path=search_paths[HOME_DIR],
+        )
+    assert actual_path == expected_path
+
+
+def test_pylint_widget_noproject(pylint_test_script, qtbot):
+    """Test that pylint works without errors with no project open."""
+    pylint_widget = PylintWidget(parent=None)
+    pylint_widget.analyze(filename=pylint_test_script)
+    qtbot.waitUntil(
+        lambda: pylint_widget.get_data(pylint_test_script)[1] is not None,
+        timeout=5000)
+    pylint_data = pylint_widget.get_data(filename=pylint_test_script)
+    print(pylint_data)
+    assert pylint_data
+    assert pylint_data[0] is not None
+    assert pylint_data[1] is not None
+
+
+@pytest.mark.skipif(PY2, reason="It fails on PY2 for no obvious reason")
+def test_pylint_widget_pylintrc(
+        pylint_test_script, pylintrc_files, mocker, qtbot):
+    """Test that entire pylint widget gets results depending on pylintrc."""
+    search_paths, __, bad_names = pylintrc_files
+    mocker.patch("pylint.config.os.path.expanduser",
+                 return_value=search_paths[HOME_DIR])
+    mocker.patch("spyder.plugins.pylint.widgets.pylintgui.getcwd_or_home",
+                 return_value=search_paths[WORKING_DIR])
+    mock_parent = mocker.Mock()
+    mock_parent.main.projects.get_active_project_path = mocker.MagicMock(
+        return_value=search_paths[PROJECT_DIR])
+    mocker.patch(
+        "spyder.plugins.pylint.widgets.pylintgui.PylintWidget.parentWidget",
+        return_value=mock_parent)
+    mocker.patch("spyder.plugins.pylint.widgets.pylintgui.osp.expanduser",
+                 return_value=search_paths[HOME_DIR])
+
+    pylint_widget = PylintWidget(parent=None)
+    pylint_widget.analyze(filename=pylint_test_script)
+    qtbot.waitUntil(
+        lambda: pylint_widget.get_data(pylint_test_script)[1] is not None,
+        timeout=5000)
+    pylint_data = pylint_widget.get_data(filename=pylint_test_script)
+    print(pylint_data)
+    assert pylint_data
+    conventions = pylint_data[1][3]["C:"]
+    assert conventions
+    assert len(conventions) == len(bad_names)
+    assert all([sum([bad_name in message[2] for message in conventions]) == 1
+                for bad_name in bad_names])
+
+
+if __name__ == "__main__":
+    pytest.main([osp.basename(__file__), '-vv', '-rw'])
diff --git a/spyder/plugins/pylint/widgets/pylintgui.py b/spyder/plugins/pylint/widgets/pylintgui.py
index 4241ae5e013..bf7733c92a8 100644
--- a/spyder/plugins/pylint/widgets/pylintgui.py
+++ b/spyder/plugins/pylint/widgets/pylintgui.py
@@ -13,6 +13,7 @@
 
 # Standard library imports
 from __future__ import print_function, with_statement
+import os
 import os.path as osp
 import re
 import sys
@@ -20,6 +21,7 @@
 
 # Third party imports
 import pylint
+import pylint.config
 from qtpy.compat import getopenfilename
 from qtpy.QtCore import QByteArray, QProcess, Signal, Slot, QProcessEnvironment
 from qtpy.QtWidgets import (QHBoxLayout, QLabel, QMessageBox, QTreeWidgetItem,
@@ -44,6 +46,31 @@
     import gettext
     _ = gettext.gettext
 
+
+def _find_pylintrc_path(path):
+    os.chdir(path)
+    return pylint.config.find_pylintrc()
+
+
+def get_pylintrc_path(search_paths=None, home_path=None):
+    """Get the path to the highest pylintrc file on a set of search paths."""
+    current_cwd = os.getcwd()
+    pylintrc_path = None
+    if home_path is None:
+        home_path = osp.expanduser("~")
+    try:
+        pylintrc_paths = [
+            _find_pylintrc_path(path) for path in search_paths if path]
+        pylintrc_path_home = _find_pylintrc_path(home_path)
+        for pylintrc_path in pylintrc_paths:
+            if (pylintrc_path is not None
+                    and pylintrc_path != pylintrc_path_home):
+                break
+    finally:
+        os.chdir(current_cwd)
+    return pylintrc_path
+
+
 PYLINT_VER = pylint.__version__
 #TODO: display results on 3 columns instead of 1: msg_id, lineno, message
 class ResultsTree(OneColumnTree):
@@ -310,6 +337,21 @@ def analyze_button_handler(self):
         """Try to start code analysis when Analyze button pressed."""
         self.start_analysis.emit()
 
+    def get_pylintrc_path(self, filename):
+        """Get the path to the most proximate pylintrc config to the file."""
+        parent = self.parentWidget()
+        if parent is not None:
+            project_dir = parent.main.projects.get_active_project_path()
+        else:
+            project_dir = None
+        search_paths = [
+            osp.dirname(filename),  # File's directory
+            getcwd_or_home(),  # Working directory
+            project_dir,  # Project directory
+            osp.expanduser("~"),  # Home directory
+        ]
+        return get_pylintrc_path(search_paths=search_paths)
+
     @Slot()
     def start(self):
         """Start the code analysis."""
@@ -317,7 +359,7 @@ def start(self):
 
         self.process = QProcess(self)
         self.process.setProcessChannelMode(QProcess.SeparateChannels)
-        self.process.setWorkingDirectory(osp.dirname(filename))
+        self.process.setWorkingDirectory(getcwd_or_home())
         self.process.readyReadStandardOutput.connect(self.read_output)
         self.process.readyReadStandardError.connect(
                                           lambda: self.read_output(error=True))
@@ -336,11 +378,13 @@ def start(self):
             else:
                 # Option '-i' (alias for '--include-ids') was removed in pylint
                 # 1.0
-                p_args += ["--msg-template='{msg_id}:{line:3d},"\
+                p_args += ["--msg-template='{msg_id}:{line:3d},"
                            "{column}: {obj}: {msg}"]
-            p_args += [osp.basename(filename)]
-        else:
-            p_args = [osp.basename(filename)]
+
+        pylintrc_path = self.get_pylintrc_path(filename=filename)
+        if pylintrc_path is not None:
+            p_args += ['--rcfile={}'.format(pylintrc_path)]
+        p_args += [filename]
         processEnvironment = QProcessEnvironment()
         processEnvironment.insert("PYTHONIOENCODING", "utf8")
         self.process.setProcessEnvironment(processEnvironment)


Code review comments:
diff_hunk: @@ -44,6 +46,31 @@
     import gettext
     _ = gettext.gettext
 
+
+def _find_pylintrc_path(path):
path: spyder/plugins/pylint/widgets/pylintgui.py
text: @user2:
@user1-Gerlach maybe this function and the one below should go to a `utils` module?

@author:
I suppose it could be, but IMO it makes much more sense to keep it somewhere inside the pylint plugin (maybe a different file if you suggest such), since its intrinsically part of the pylint plugin's implementation, not an external/general use utility. Furthermore, it would introduce complications, confusion and extra work for testing since it would either bifurcate the test code testing essentially the same thing at two different levels of abstraction, or require testing an external utility inside a plugin's tests.

@user2:
Yep, when I said a `utils` module I was thinking in a new file inside the `pylint` directory. Maybe following a directory structure like:
```
spyder/plugins/pylint/utils/path.py
```

@author:
Oh, sorry, that makes sense then. I would suggest `spyder/plugins/pylint/utils/pylintrc.py` if design, spec or policy prohibits making it just a `utils` module in the base `pylint` dir. Given the relatively short length and unlikelihood of much else being added about path/pylintrc specifically as opposed to the higher possibility of more general pylint utils, for what its worth I would think it best to avoid adding yet another level of nesting to an already very deeply nested python package (given PEP 8 and `import this` (the Zen of Python) both strongly urge for keeping the namespace as flat as practical). However, this may be an consequence of the Spyder plugin spec or similar.

@user3 What do you suggest we go with? Unless you or someone else has anything additional, this should be the last change needed before this one can be merged for 4.1.0.

@author:
@user4 You may want to chime in too.

@user4:
I would do something like: 

`spyder/plugins/pylint/utils.py`

@author:
Yeah, that was my preference too as I mentioned, I just wasn't sure if the Spyder plugin spec or policy/convention prohibited it based on @user2 not suggesting it himself. I'll implement it now.

@author:
Implemented and pushed now, plus some more minor formatting cleanup and a few more code comments. Once test past (as they did again locally) this should be ready on my end unless anyone has more suggestions/followups.

diff_hunk: @@ -44,6 +46,31 @@
     import gettext
     _ = gettext.gettext
 
+
+def _find_pylintrc_path(path):
+    os.chdir(path)
+    return pylint.config.find_pylintrc()
+
+
+def get_pylintrc_path(search_paths=None, home_path=None):
+    """Get the path to the highest pylintrc file on a set of search paths."""
+    current_cwd = os.getcwd()
+    pylintrc_path = None
+    if home_path is None:
+        home_path = osp.expanduser("~")
+    try:
+        pylintrc_paths = [
+            _find_pylintrc_path(path) for path in search_paths if path]
+        pylintrc_path_home = _find_pylintrc_path(home_path)
+        for pylintrc_path in pylintrc_paths:
+            if (pylintrc_path is not None
+                    and pylintrc_path != pylintrc_path_home):
+                break
+    finally:
path: spyder/plugins/pylint/widgets/pylintgui.py
text: @user1:
Maybe should we add here an except statement to log the exception that could raise and properly show the kind of exception that could raise here?

@author:
A fair point. We don't expect any particular exception here, this is only for safety so that the working directory is reset properly in case of an unexpected error, following the general principle of not modifying de-facto global state unless necessary. Two possible scenarios do come to mind: First, pylint not being installed, which would raise (and presumably handle, e.g. by displaying a user warning or disabling the plugin) many errors long before this in the code; the error would in fact the error would presumably occur on import of the pylint module, which would be the appropriate place to handle it if we want to if it is not handled with a warning message on startup, not here.

Second if some new major version of pylint were to move, rename or remove the `pylint.config.find_pylintrc()` function, which we _want_ to fail with an actual error, not just a subtle log message, so that we see it immediately on our CIs before users do as soon as such an update goes out and gets pulled in by Spyder, and if for whatever reason users _do_ see it first, it is handled by Spyder's normal error report dialog so it gets reported so we can see and fix it, not buried in a local log somewhere.

diff_hunk: @@ -44,6 +46,31 @@
     import gettext
     _ = gettext.gettext
 
+
+def _find_pylintrc_path(path):
+    os.chdir(path)
+    return pylint.config.find_pylintrc()
+
+
+def get_pylintrc_path(search_paths=None, home_path=None):
+    """Get the path to the highest pylintrc file on a set of search paths."""
+    current_cwd = os.getcwd()
+    pylintrc_path = None
+    if home_path is None:
+        home_path = osp.expanduser("~")
+    try:
+        pylintrc_paths = [
+            _find_pylintrc_path(path) for path in search_paths if path]
+        pylintrc_path_home = _find_pylintrc_path(home_path)
+        for pylintrc_path in pylintrc_paths:
+            if (pylintrc_path is not None
+                    and pylintrc_path != pylintrc_path_home):
+                break
path: spyder/plugins/pylint/widgets/pylintgui.py
text: @user1:
What is the part in this code that we are trying to catch?

Ideally try/catch should be as small as possible to be very granular about what we are catching.

@author:
Its not excepting ("catching") anything, as if you look at the function this isn't a try-except at all, its a try-finally, in order to ensure the working directory gets set back to what it was prior to the function's execution. I normally include an explanatory comment documenting the purpose of any such similar construct, but I must not have added when I originally wrote the code since I didn't intend to implement it myself. I've added it now for the next commit.