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

fix(client/linters/mypy): call mypy incorrectly #5834

Merged
merged 5 commits into from
Nov 11, 2019

Conversation

yxliang01
Copy link

@yxliang01 yxliang01 commented May 29, 2019

It's supposed to call mypy in the root package directory with the relative path to the python file being checked. This commit enforces this behavior.

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Appropriate comments and documentation strings in the code
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated
  • Test plan is updated as appropriate
  • package-lock.json has been regenerated by running npm install (if dependencies have changed)
  • The wiki is updated with any design decisions/details.

Before this PR is merged, you may use https://gist.github.com/yxliang01/b975fffa0b145d75ed1dba5b3a9ad960 as the mypy wrapper to achieve the same effect.

Thanks @harpaj for the idea.
Related to #5326

It's supposed to call `mypy` in the root package directory with the relative path to the python file being checked. This commit enforces this behavior.
@yxliang01
Copy link
Author

I couldn't really think of any suitable test case for this. Any suggestion? Or, can I just mark test case check as checked?

@DonJayamanne
Copy link

Apologies for never getting back to you on this issue. Somehow we missed this.
I can add the necessary tests.
And as always, thank you very much for the fix.

@DonJayamanne
Copy link

@yxliang01 I'll add the tests for the PR.
Let me try to merge this and check (closing to restart CI)

@DonJayamanne
Copy link

DonJayamanne commented Jun 25, 2019

@yxliang01
Thanks for this PR. I'd like to merge this in, however the tests are failing, please could you fix the failing tests.
I can add new tests for the code you have written or I can share them with you (I'm unable to push my changes up to your branch).

Here are the tests (You can add them Or i can after I merge your PR) DonJayamanne@9225b6e

@DonJayamanne DonJayamanne removed their assignment Sep 28, 2019
@yxliang01
Copy link
Author

@DonJayamanne Hmm wondering how this going?

@DonJayamanne
Copy link

@karthiknadig /cc

@karthiknadig karthiknadig self-assigned this Nov 9, 2019
@msftclas
Copy link

msftclas commented Nov 9, 2019

CLA assistant check
All CLA requirements met.

@karthiknadig
Copy link
Member

@yxliang01 I added the tests and news item. You have to sign the CLA before i can merge.

@yxliang01
Copy link
Author

@DonJayamanne @karthiknadig CLA signed :)

@karthiknadig karthiknadig merged commit 1b6fbfb into microsoft:master Nov 11, 2019
@yxliang01 yxliang01 deleted the patch-1 branch November 11, 2019 18:40
@yxliang01 yxliang01 restored the patch-1 branch November 11, 2019 18:40
@lock lock bot locked as resolved and limited conversation to collaborators Nov 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants