Skip to content

Conversation

@matthewshirley
Copy link

@matthewshirley matthewshirley commented Jan 19, 2021

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

This may need to change after #15266 is merged.

Copy link

@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.

@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

bl-ue commented Mar 23, 2021

Any progress? Dying for this feature.

@karrtikr
Copy link

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

@matthewshirley
Copy link
Author

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

Copy link

@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.

Please have a look at

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.

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 requested review from karrtikr and kimadeline April 5, 2021 22:47
Copy link

@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!

Copy link

@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

bl-ue commented Apr 7, 2021

🚀 🎊 🎉

@lg-kialo
Copy link

Thank you so much for this!

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose a setting to change the working directory of linters

5 participants