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

Added workaround and early returning if file name matches excluded arg #142

Merged

Conversation

kirankumarmanku
Copy link
Contributor

In this PR, I am checking if the file name matches in excluded args in settings file and doing an early return.

The check is similar to how autopep8 does its matching here: https://github.com/hhatto/autopep8/blob/5b9110ba53fecd60cd3091fb66d808e8ced3b2e8/autopep8.py#L4359

@kirankumarmanku
Copy link
Contributor Author

@microsoft-github-policy-service agree

@karthiknadig karthiknadig added the bug Issue identified by VS Code Team member as probable bug label Aug 8, 2023
@karthiknadig karthiknadig self-assigned this Aug 8, 2023
@karthiknadig
Copy link
Member

@kirankumarmanku This particular scenario requires its own test. Add test here https://github.com/microsoft/vscode-autopep8/blob/main/src/test/python_tests/test_formatting.py

You can update arguments passed to the server like this:

    with session.LspSession() as ls_session:
        init_options = defaults.VSCODE_DEFAULT_INITIALIZE["initializationOptions"]
        init_options["settings"][0]["args"] = ["--exclude", "<pattern>"]
        ls_session.initialize(defaults.VSCODE_DEFAULT_INITIALIZE)

@kirankumarmanku
Copy link
Contributor Author

@karthiknadig ,
I addressed the comment and added couple of test cases testing main scenario. Let me know if it looks good.

src/test/python_tests/test_formatting.py Outdated Show resolved Hide resolved
src/test/python_tests/test_formatting.py Outdated Show resolved Hide resolved
src/test/python_tests/test_formatting.py Outdated Show resolved Hide resolved
src/test/python_tests/test_formatting.py Outdated Show resolved Hide resolved
@karthiknadig
Copy link
Member

The uri that is sent in ls_session.text_document_formatting is what you will get for fnmatch. So, you need to make sure that the URI that is passed there is the exact one you are testing.

@karthiknadig
Copy link
Member

If you have not set up the repo for development follow the steps here: https://github.com/microsoft/vscode-autopep8/wiki/Contributing-Guide

Once you have it set up you should see tests in the test explorer:
image

You can set breakpoint in python code and click on this button for your test case:
image

@kirankumarmanku
Copy link
Contributor Author

@karthiknadig ,
Thanks for the suggestion. Turns out, we are changing the file name before sending it to lsp_server in test cases, that's why test case is failing to detect the excluded file. Hope we don't do this in actual execution and document.path points to the path name of the file we are editing.

@kirankumarmanku
Copy link
Contributor Author

@karthiknadig,
I updated the PR with the changes you suggested. Can you check and let me know if anything needs to be done?

karthiknadig
karthiknadig previously approved these changes Aug 9, 2023
src/test/python_tests/test_formatting.py Outdated Show resolved Hide resolved
@karthiknadig
Copy link
Member

You might need to add sample_formatter.py to the skip list for isort. You can run nox --session lint` to check this locally.

@kirankumarmanku
Copy link
Contributor Author

I updated the PR @karthiknadig. Please take a look!

@kirankumarmanku
Copy link
Contributor Author

@karthiknadig , it seems I need one more approval from the Community. How do I get that?

@kirankumarmanku
Copy link
Contributor Author

@karthiknadig , @lramos15 Thank you for approving the PR. Can you also merge this? I don't have write access for the repository to do that.

@karthiknadig karthiknadig merged commit 8d97447 into microsoft:main Aug 9, 2023
18 checks passed
@kirankumarmanku kirankumarmanku deleted the workaround-for-exclude-arg branch August 9, 2023 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants