Skip to content

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

Merged
karthiknadig merged 9 commits intomicrosoft:mainfrom
kirankumarmanku:workaround-for-exclude-arg
Aug 9, 2023
Merged

Added workaround and early returning if file name matches excluded arg#142
karthiknadig merged 9 commits intomicrosoft:mainfrom
kirankumarmanku:workaround-for-exclude-arg

Conversation

@kirankumarmanku
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

Comment thread src/test/python_tests/test_formatting.py Outdated
Comment thread src/test/python_tests/test_formatting.py Outdated
Comment thread src/test/python_tests/test_formatting.py Outdated
Comment thread src/test/python_tests/test_formatting.py Outdated
@karthiknadig
Copy link
Copy Markdown
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.

Comment thread bundled/tool/lsp_server.py Outdated
@karthiknadig
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Comment thread src/test/python_tests/test_formatting.py Outdated
@karthiknadig
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@kirankumarmanku
Copy link
Copy Markdown
Contributor Author

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

@kirankumarmanku
Copy link
Copy Markdown
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
@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.

3 participants