Skip to content
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

exclude option does not follow symlinks when generating exclude_set for relative paths in Python <3.13 #457

Closed
jeancochrane opened this issue Jul 27, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@jeancochrane
Copy link
Contributor

Describe the bug

Given the following directory structure:

testdir/
    subdir/
        include_me.sql
    venv/
        symlink/ => ../../sourcedir/
sourcedir/
    exclude_me.sql

If we try to run sqlfmt testdir --exclude "testdir/venv/**/*", testdir/venv/symlink/exclude_me.sql will not be excluded, due to a known bug in Python <3.13 that prevents pathlib.Path.glob() from following symlinks in this code block:

sqlfmt/src/sqlfmt/api.py

Lines 111 to 114 in abf9763

try:
exclude_set.update(Path.cwd().glob(s))
except IndexError:
pass

This is particularly a problem for directories containing Python virtual environments, where lib64/ is often a symlink to lib/ and can contain third-party packages that should be excluded by sqlfmt.

To Reproduce

Create the above directory structure:

mkdir -p testdir/subdir/
touch testdir/subdir/include_me.sql
mkdir sourcedir
touch sourcedir/exclude_me.sql
mkdir testdir/venv
ln -s "`pwd`/sourcedir" testdir/venv/symlink

Run sqlfmt and try to exclude testdir/venv/**/*:

$ sqlfmt testdir/ -v --check --exclude "testdir/venv/**/*"

Expected behavior

With the above directory structure, sqlfmt should follow the symlink and exclude exclude_me.sql:

$ sqlfmt testdir/ -v --check --exclude "testdir/venv/**/*"
1 file passed formatting check.
testdir/subdir/include_me.sql passed formatting check.

Actual behavior

sqlfmt includes exclude_me.sql:

$ sqlfmt testdir/ -v --check --exclude "testdir/venv/**/*"
2 files passed formatting check.
testdir/subdir/include_me.sql passed formatting check.
testdir/venv/symlink/exclude_me.sql passed formatting check.

Additional context

I'm not totally sure what the right way to handle this is, given that the underlying Path.glob() bug will be fixed in the upcoming Python 3.13 release. It seems like the choice that would support a wider range of versions would be to replace the Path.glob() call with a relative glob.glob() call, which seems to be a common workaround, but I don't know how high priority it is to support this behavior. If you agree with switching to glob.glob(), I'd be happy to put up a patch!

@tconbeer
Copy link
Owner

Didn't realize that; 3.13 seems pretty far away, so I'd love a patch and a test if you could! Feel free to reach out if you need any help.

Similar but probably not related to #440

I'm fine if you want to leave Windows symlinks for another issue.

@tconbeer tconbeer added the bug Something isn't working label Jul 27, 2023
@jeancochrane
Copy link
Contributor Author

Thanks @tconbeer, I'll aim to make some time for a patch this week!

@tconbeer
Copy link
Owner

@jeancochrane Patch has been released in v0.19.2. Thanks so much for contributing to sqlfmt!

@jeancochrane
Copy link
Contributor Author

Thanks for the quick turnaround @tconbeer, my team really appreciates it!

@tconbeer
Copy link
Owner

You made it very easy for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants