Skip to content

Expose a setting to allow a user modify the working directory of linters#15171

Merged
kimadeline merged 10 commits into
microsoft:mainfrom
matthewshirley:issue-15170/cwd-for-linting
Apr 6, 2021
Merged

Expose a setting to allow a user modify the working directory of linters#15171
kimadeline merged 10 commits into
microsoft:mainfrom
matthewshirley:issue-15170/cwd-for-linting

Conversation

@matthewshirley
Copy link
Copy Markdown

@matthewshirley matthewshirley commented Jan 19, 2021 •
edited by kimadeline
Loading

Closes #15170

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • The wiki is updated with any design decisions/details.

Labels

Needs skip package*.json

Tests

I'm uncertain where to introduce tests for this PR. I couldn't identify where it would fit. Let me know if they're need and the best suite for them. 💪

@karrtikr
Copy link
Copy Markdown

karrtikr commented Feb 24, 2021

This may need to change after #15266 is merged.

kimadeline
kimadeline suggested changes Mar 16, 2021
View reviewed changes
Copy link
Copy Markdown

@kimadeline kimadeline left a comment

Choose a reason for hiding this comment

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

Hi @matthewshirley, thank you for your contribution!

Yep please add tests, src/test/linters/lint.functional.test.ts would be a good spot for them. Adding them to the existing suite should be fine.

I left a couple of comments regarding the type of the python.linting.cwd setting, they apply to src/test/linters/common.ts and src/test/mockClasses.ts as well.

Comment thread src/client/common/configSettings.ts Outdated
Comment thread src/client/common/types.ts Outdated
@kimadeline kimadeline added the skip package*.json package.json and package-lock.json don't both need updating label Mar 16, 2021
@bl-ue
Copy link
Copy Markdown

bl-ue commented Mar 23, 2021

Any progress? Dying for this feature.

@karrtikr
Copy link
Copy Markdown

karrtikr commented Mar 29, 2021

@bl-ue Waiting on @matthewshirley to address the comments.

@matthewshirley
Copy link
Copy Markdown
Author

matthewshirley commented Mar 29, 2021

I will respond and work on tests for the PR this week. 👍 Thanks for the patience.

karrtikr
karrtikr suggested changes Mar 29, 2021
View reviewed changes
Copy link
Copy Markdown

@karrtikr karrtikr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please have a look at

vscode-python/src/client/linters/baseLinter.ts

Line 98 in e3e773a

protected getWorkspaceRootPath(document: vscode.TextDocument): string {

Attempt to find all references to the method.

It looks like we assume that is the cwd in these places. For eg.

vscode-python/src/client/linters/prospector.ts

Line 33 in db8e1e2

const cwd = this.getWorkspaceRootPath(document);

I think you'll need to adjust that method to return the correct cwd using the setting instead of the workspace root, and use the workspace root as default cwd if not set.

karrtikr
karrtikr reviewed Mar 29, 2021
View reviewed changes
Comment thread news/1 Enhancements/15170.md Outdated
karrtikr
karrtikr reviewed Apr 2, 2021
View reviewed changes
Comment thread src/client/linters/baseLinter.ts
@karrtikr karrtikr requested review from karrtikr and kimadeline April 5, 2021 22:47
karrtikr
karrtikr approved these changes Apr 5, 2021
View reviewed changes
Copy link
Copy Markdown

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

kimadeline
kimadeline approved these changes Apr 6, 2021
View reviewed changes
Copy link
Copy Markdown

@kimadeline kimadeline left a comment

Choose a reason for hiding this comment

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

Thank you again for your contribution, @matthewshirley! 🥳

@kimadeline kimadeline merged commit bbdba53 into microsoft:main Apr 6, 2021
@bl-ue
Copy link
Copy Markdown

bl-ue commented Apr 7, 2021

🚀 🎊 🎉

@lg-kialo
Copy link
Copy Markdown

lg-kialo commented Sep 10, 2021

Thank you so much for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@karrtikr karrtikr karrtikr approved these changes

@kimadeline kimadeline kimadeline approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kimadeline kimadeline

Labels

skip package*.json package.json and package-lock.json don't both need updating

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Expose a setting to change the working directory of linters

5 participants

@matthewshirley @karrtikr @bl-ue @lg-kialo @kimadeline