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

Change file paths to use an uppercase drive letter when normalized #111

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

mhahn-ts
Copy link
Contributor

When using the extension to lint, it would neglect to use the .sqlfluffignore file in the root of my project. According to the SQLFluff Docs, this is where the file should be placed.

I found that the issue would be resolved if I set the sqlfluff.workingDirectory using a capital C - C:\dev\MyProject. If I used a lowercase c - c:\dev\MyProject - it would not work. While debugging, I noticed that omitting the sqlfluff.workingDirectory or using a variable (such as ${workspaceFolder}) would result in a lowercase c being used as the drive. This aligns with the VS Code API Docs for fspath. It always returns a lowercase drive letter.

@RobertOstermann
Copy link
Contributor

@mhahn-ts Are you using Windows? I didn't think the Windows filesystem was case-sensitive, so not sure why having a lowercase drive would matter?

@mhahn-ts
Copy link
Contributor Author

mhahn-ts commented Aug 23, 2023

@mhahn-ts Are you using Windows? I didn't think the Windows filesystem was case-sensitive, so not sure why having a lowercase drive would matter?

Yes, I'm on Windows. I am able to reproduce with just SQLFluff in the command line. In fact, it seems to treat the entire filepath in a case-sensitive manner.
image

In my opinion, it's an issue with SQLFluff. I was planning to create an issue in that repo. This is especially true since you cannot specify absolute path in the .sqlfluffignore. You must just give relative path.
But, based on how the extension automatically gets that ${workspaceFolder}, I do think it is still appropriate to fix it here. If SQLFluff requires a case-sensitive matched filepath, we should make sure that the filepath variables are created that way.

I'd be happy to add more comments or throw it behind a config setting if you think that's appropriate, too.

@RobertOstermann
Copy link
Contributor

Interesting, I just assumed it would work with lowercase drive letters. Thanks for looking into this. I don't think it is necessary to put it behind a config setting, your changes should be good. I should have time tonight to get them merged and publish a release with the changes.

@RobertOstermann RobertOstermann merged commit 5bd2cbf into sqlfluff:master Aug 23, 2023
5 checks passed
@RobertOstermann
Copy link
Contributor

@mhahn-ts This has been released in v2.4.3

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.

2 participants