sqlfluff · alanmcruickshank · Nov 21, 2024 · Oct 24, 2024 · Oct 24, 2024 · Oct 24, 2024
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
@@ -45,7 +45,6 @@ repos:
           [
             types-toml,
             types-chardet,
-            types-appdirs,
             types-colorama,
             types-pyyaml,
             types-regex,
@@ -59,6 +58,7 @@ repos:
             pathspec,
             pytest, # and by extension... pluggy
             click,
+            platformdirs
           ]
         files: ^src/sqlfluff/.*
         # The mypy pre-commit hook by default sets a few arguments that we don't normally

diff --git a/pyproject.toml b/pyproject.toml
@@ -67,7 +67,7 @@ keywords = [
 ]
 dependencies = [
     # Used for finding os-specific application config dirs
-    "appdirs",
+    "platformdirs",
     # To get the encoding of files.
     "chardet",
     "click",

diff --git a/src/sqlfluff/core/config/loader.py b/src/sqlfluff/core/config/loader.py
@@ -17,12 +17,15 @@
 import logging
 import os
 import os.path
+import sys
 from pathlib import Path
 from typing import (
     Optional,
 )
 
-import appdirs
+import platformdirs
+import platformdirs.macos
+import platformdirs.unix
 
 from sqlfluff.core.config.file import (
     cache,
@@ -55,22 +58,50 @@
 )
 
 
-def _get_user_config_dir_path() -> str:
+def _get_user_config_dir_path(sys_platform: str) -> str:
+    """Get the user config dir for this system.
+
+    Args:
+        sys_platform (str): The result of ``sys.platform()``. Provided
+            as an argument here for ease of testing. In normal usage
+            it should only be  called with ``sys.platform()``. This
+            argument only applies to switching between linux and macos.
+            Win32 detection still uses the underlying ``sys.platform()``
+            methods.
+    """
     appname = "sqlfluff"
     appauthor = "sqlfluff"
 
-    # On Mac OSX follow Linux XDG base dirs
-    # https://github.com/sqlfluff/sqlfluff/issues/889
-    user_config_dir_path = os.path.expanduser("~/.config/sqlfluff")
-    if appdirs.system == "darwin":
-        appdirs.system = "linux2"
-        user_config_dir_path = appdirs.user_config_dir(appname, appauthor)
-        appdirs.system = "darwin"
-
-    if not os.path.exists(user_config_dir_path):
-        user_config_dir_path = appdirs.user_config_dir(appname, appauthor)
+    # First try the default SQLFluff specific cross-platform config path.
+    cross_platform_path = os.path.expanduser("~/.config/sqlfluff")
+    if os.path.exists(cross_platform_path):
+        return cross_platform_path
 
-    return user_config_dir_path
+    # Then try the platform specific paths, for MacOS, we check
+    # the unix variant first to preferentially use the XDG config path if set.
+    # https://github.com/sqlfluff/sqlfluff/issues/889
+    if sys_platform == "darwin":
+        unix_config_path = platformdirs.unix.Unix(
+            appname=appname, appauthor=appauthor
+        ).user_config_dir
+        if os.path.exists(os.path.expanduser(unix_config_path)):
+            return unix_config_path
+        # Technically we could just delegate to the generic `user_config_dir`
+        # method, but for testing it's convenient to explicitly call the macos
+        # methods here.
+        return platformdirs.macos.MacOS(
+            appname=appname, appauthor=appauthor
+        ).user_config_dir
    # NOTE: We could delegate to the generic `user_config_dir` method here,
+    # but for testing it's convenient to explicitly call the linux methods.
+    elif sys_platform == "linux":
+        return platformdirs.unix.Unix(
+            appname=appname, appauthor=appauthor
+        ).user_config_dir
+    # Defer to the self-detecting paths.
+    # NOTE: On Windows this means that the `sys_platform` argument is not
+    # applied.
+    return platformdirs.user_config_dir(appname, appauthor)
 
 
 def load_config_file(
@@ -218,7 +249,7 @@ def load_config_at_path(path: str) -> ConfigMappingType:
 
 def _load_user_appdir_config() -> ConfigMappingType:
     """Load the config from the user's OS specific appdir config directory."""
-    user_config_dir_path = _get_user_config_dir_path()
+    user_config_dir_path = _get_user_config_dir_path(sys.platform)
     if os.path.exists(user_config_dir_path):
         return load_config_at_path(user_config_dir_path)
     else:
@@ -283,16 +314,19 @@ def load_config_up_to_path(
         config_paths = iter_intermediate_paths(Path(path).absolute(), Path.cwd())
         config_stack = [load_config_at_path(str(p.resolve())) for p in config_paths]
 
-    # 4) Extra config paths
-    if not extra_config_path:
-        extra_config = {}
-    else:
-        if not os.path.exists(extra_config_path):
+    # 4) Extra config paths.
+    # When calling `load_config_file_as_dict` we resolve the path first so that caching
+    # is more efficient.
+    extra_config = {}
+    if extra_config_path:
+        try:
+            extra_config = load_config_file_as_dict(
+                str(Path(extra_config_path).resolve())
+            )
+        except FileNotFoundError:
             raise SQLFluffUserError(
-                f"Extra config '{extra_config_path}' does not exist."
+                f"Extra config path '{extra_config_path}' does not exist."
             )
-        # Resolve the path so that the caching is accurate.
-        extra_config = load_config_file_as_dict(str(Path(extra_config_path).resolve()))
 
     return nested_combine(
         user_appdir_config,

diff --git a/test/cli/commands_test.py b/test/cli/commands_test.py
@@ -228,8 +228,8 @@ def test__cli__command_extra_config_fail():
             ],
         ],
         assert_output_contains=(
-            "Extra config 'test/fixtures/cli/extra_configs/.sqlfluffsdfdfdfsfd' does "
-            "not exist."
+            "Extra config path 'test/fixtures/cli/extra_configs/.sqlfluffsdfdfdfsfd' "
+            "does not exist."
         ),
     )
 

diff --git a/test/core/config/loader_test.py b/test/core/config/loader_test.py
@@ -5,7 +5,6 @@
 from contextlib import contextmanager
 from unittest.mock import call, patch
 
-import appdirs
 import pytest
 
 from sqlfluff.core import FluffConfig
@@ -19,6 +18,7 @@
     _get_user_config_dir_path,
     _load_user_appdir_config,
 )
+from sqlfluff.core.errors import SQLFluffUserError
 
 config_a = {
     "core": {"testing_val": "foobar", "testing_int": 4, "dialect": "mysql"},
@@ -59,21 +59,43 @@ def test__config__load_file_f():
     assert cfg == config_a
 
 
+def test__config__load_file_missing_extra():
+    """Test loading config from a file path if extra path is not found."""
+    with pytest.raises(SQLFluffUserError):
+        load_config_up_to_path(
+            os.path.join("test", "fixtures", "config", "inheritance_a", "testing.sql"),
+            extra_config_path="non/existent/path",
+        )
+
+
 def test__config__load_nested():
     """Test nested overwrite and order of precedence of config files."""
     cfg = load_config_up_to_path(
         os.path.join(
             "test", "fixtures", "config", "inheritance_a", "nested", "blah.sql"
-        )
+        ),
+        extra_config_path=os.path.join(
+            "test",
+            "fixtures",
+            "config",
+            "inheritance_a",
+            "extra",
+            "this_can_have_any_name.cfg",
+        ),
     )
     assert cfg == {
         "core": {
+            # Outer .sqlfluff defines dialect & testing_val and not overridden.
             "dialect": "mysql",
             "testing_val": "foobar",
+            # tesing_int is defined in many. Inner pyproject.toml takes precedence.
             "testing_int": 1,
+            # testing_bar is defined only in setup.cfg
             "testing_bar": 7.698,
         },
-        "bar": {"foo": "foobar"},
+        # bar is defined in a few, but the extra_config takes precedence.
+        "bar": {"foo": "foobarextra"},
+        # fnarr is defined in a few. Inner tox.ini takes precedence.
         "fnarr": {"fnarr": {"foo": "foobar"}},
     }
 
@@ -158,37 +180,107 @@ def test__config__load_placeholder_cfg():
 @patch("os.path.exists")
 @patch("os.listdir")
 @pytest.mark.skipif(sys.platform == "win32", reason="Not applicable on Windows")
-def test__config__load_user_appdir_config(
-    mock_listdir, mock_path_exists, mock_xdg_home
+@pytest.mark.parametrize(
+    "sys_platform,xdg_exists,default_exists,resolved_config_path,paths_checked",
+    [
+        # On linux, if the default path exists, it should be the only path we check
+        # and the chosen config path.
+        ("linux", True, True, "~/.config/sqlfluff", ["~/.config/sqlfluff"]),
+        # On linux, if the default path doesn't exist, then (because for this
+        # test case we set XDG_CONFIG_HOME) it will check the default path
+        # but then on finding it to not exist it will then try the XDG path.
+        # In this case, neither actually exist and so what matters is that both
+        # are either checked or used - rather than one in particular being the
+        # end result.
+        (
+            "linux",
+            False,
+            False,
+            "~/.config/my/special/path/sqlfluff",
+            ["~/.config/sqlfluff"],
+        ),
+        # On MacOS, if the default config path and the XDG path don't exist, then
+        # we should resolve config to the default MacOS config path.
+        (
+            "darwin",
+            False,
+            False,
+            "~/Library/Application Support/sqlfluff",
+            ["~/.config/sqlfluff", "~/.config/my/special/path/sqlfluff"],
+        ),
+        # However, if XDG_CONFIG_HOME is set, and the path exists then that should
+        # be resolved _ahead of_ the default MacOS config path (as demonstrated
+        # by us not checking the presence of that path in the process).
+        # https://github.com/sqlfluff/sqlfluff/issues/889
+        (
+            "darwin",
+            True,
+            False,
+            "~/.config/my/special/path/sqlfluff",
+            ["~/.config/sqlfluff", "~/.config/my/special/path/sqlfluff"],
+        ),
+    ],
+)
+def test__config__get_user_config_dir_path(
+    mock_listdir,
+    mock_path_exists,
+    mock_xdg_home,
+    sys_platform,
+    xdg_exists,
+    default_exists,
+    resolved_config_path,
+    paths_checked,
 ):
     """Test loading config from user appdir."""
     xdg_home = os.environ.get("XDG_CONFIG_HOME")
     assert xdg_home, "XDG HOME should be set by the mock. Something has gone wrong."
     xdg_config_path = xdg_home + "/sqlfluff"
 
-    def path_exists(x):
-        if x == os.path.expanduser("~/.config/sqlfluff"):
+    def path_exists(check_path):
+        """Patch for os.path.exists which depends on test parameters.
+
+        Returns:
+            True, unless `default_exists` is `False` and the path passed to
+            the function is the default config path, or unless `xdg_exists`
+            is `False` and the path passed is the XDG config path.
+        """
+        resolved_path = os.path.expanduser(check_path)
+        if (
+            resolved_path == os.path.expanduser("~/.config/sqlfluff")
+            and not default_exists
+        ):
             return False
-        if x == xdg_config_path:
+        if resolved_path == os.path.expanduser(xdg_config_path) and not xdg_exists:
             return False
-        else:
-            return True
+        return True
 
     mock_path_exists.side_effect = path_exists
 
-    with patch.object(appdirs, attribute="system", new="darwin"):
-        resolved_path = _get_user_config_dir_path()
-        _load_user_appdir_config()
-    assert resolved_path == os.path.expanduser("~/Library/Application Support/sqlfluff")
-
+    # Get the config path as though we are on macOS.
+    resolved_path = _get_user_config_dir_path(sys_platform)
+    assert os.path.expanduser(resolved_path) == os.path.expanduser(resolved_config_path)
     mock_path_exists.assert_has_calls(
-        [
-            call(xdg_config_path),
-            call(os.path.expanduser("~/Library/Application Support/sqlfluff")),
-        ]
+        [call(os.path.expanduser(path)) for path in paths_checked]
     )
 
 
+@patch("os.path.exists")
+@patch("sqlfluff.core.config.loader.load_config_at_path")
+def test__config__load_user_appdir_config(mock_load_config, mock_path_exists):
+    """Test _load_user_appdir_config.
+
+    NOTE: We mock `load_config_at_path()` so we can be really focussed with this test
+    and also not need to actually interact with local home directories.
+    """
+    mock_load_config.side_effect = lambda x: {}
+    mock_path_exists.side_effect = lambda x: True
+    _load_user_appdir_config()
+    # It will check that the default config path exists...
+    mock_path_exists.assert_has_calls([call(os.path.expanduser("~/.config/sqlfluff"))])
+    # ...and assuming it does, it will try and load config files at that path.
+    mock_load_config.assert_has_calls([call(os.path.expanduser("~/.config/sqlfluff"))])
+
+
 def test__config__toml_list_config():
     """Test Parsing TOML list of values."""
     loaded_config = load_config_file(

diff --git a/test/fixtures/config/inheritance_a/extra/this_can_have_any_name.cfg b/test/fixtures/config/inheritance_a/extra/this_can_have_any_name.cfg
@@ -0,0 +1,2 @@
+[sqlfluff:bar]
+foo=foobarextra