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

fix(config): handle mixed relative and absolute paths #427

Merged

Conversation

smcgivern
Copy link
Contributor

@smcgivern smcgivern commented May 29, 2023

We need to call absolute() to convert a relative path to an absolute path before trying to compare parents, as parents is (per the docs), 'a purely lexical operation'.

Fixes #426.

Comment on lines +70 to +72
# Only check the cases where we are providing more than one path
if len(files_relpath) == 1:
return
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure there's a better way to do this 😬

@smcgivern smcgivern force-pushed the fix-mixed-relative-and-absolute-paths branch from 1c7a9ab to d387bc1 Compare May 30, 2023 09:00
@smcgivern
Copy link
Contributor Author

The failure on Windows was because resolve() can be confusing there (at least, it was to me). I think absolute() is fine for this case - see python/cpython#26153 - so let's see if that makes CI happy.

We need to call `absolute()` to convert a relative path to an absolute
path before trying to compare parents, as `parents` is (per the docs),
'a purely lexical operation'.
@smcgivern smcgivern force-pushed the fix-mixed-relative-and-absolute-paths branch from d387bc1 to b4c9620 Compare May 30, 2023 09:31
@smcgivern
Copy link
Contributor Author

I'm assuming the coverage failure is because this PR is from a fork?

Error: you must supply a CC_TEST_REPORTER_ID ENV variable or pass it via the -r flag

@tconbeer
Copy link
Owner

I'm assuming the coverage failure is because this PR is from a fork?

Error: you must supply a CC_TEST_REPORTER_ID ENV variable or pass it via the -r flag

yes, don't worry about that one

@tconbeer
Copy link
Owner

@smcgivern Thanks so much for contributing!

@tconbeer
Copy link
Owner

tconbeer commented Jun 8, 2023

This turned out to be the wrong solution; click resolves paths before passing them into cli.main, so the rest of the code doesn't need to handle relative paths for files. We just weren't handling the case where there are no common parents. This isn't possible on posix systems, but it is on Windows.

@smcgivern
Copy link
Contributor Author

I am on macOS and sqlfmt foo $PWD/foo crashes without something like this, so I'm not sure paths are resolved going in. I mean, you can also try the test here without this change and see if it passes 😃

However, I see what you mean about click. You can set resolve_path (https://click.palletsprojects.com/en/8.1.x/api/#click.Path), but it defaults to off, and it doesn't look like it's set here?

type=click.Path(exists=True, allow_dash=True, path_type=Path),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ValueError: max() arg is an empty sequence
2 participants