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

ApplyTypeAnnotationsVisitor: fix default value #314

Merged
merged 2 commits into from
Jun 16, 2020

Conversation

hauntsaninja
Copy link
Contributor

Summary

ApplyTypeAnnotationsVisitor erases default values of positional-only and keyword-only args. See Instagram/MonkeyType#198 for a repro

Test Plan

Added tests for the relevant issues in test_apply_type_annotations.py

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 16, 2020
@codecov-commenter
Copy link

Codecov Report

Merging #314 into master will decrease coverage by 0.01%.
The diff coverage is 62.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #314      +/-   ##
==========================================
- Coverage   93.96%   93.95%   -0.02%     
==========================================
  Files         223      223              
  Lines       21438    21446       +8     
==========================================
+ Hits        20145    20150       +5     
- Misses       1293     1296       +3     
Impacted Files Coverage Δ
libcst/codemod/visitors/_apply_type_annotations.py 95.83% <ø> (ø)
...emod/visitors/tests/test_apply_type_annotations.py 89.65% <62.50%> (-10.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c023fa7...5efa834. Read the comment docs.

Copy link
Contributor

@jimmylai jimmylai left a comment

Choose a reason for hiding this comment

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

LGTM
@hauntsaninja Nice catch and thanks for the fix!

@jimmylai jimmylai merged commit 5992a7d into Instagram:master Jun 16, 2020
@hauntsaninja hauntsaninja deleted the fix branch June 16, 2020 17:33
@hauntsaninja
Copy link
Contributor Author

Thanks for the quick review! I was wondering what LibCST's release process is like? This feels like a fairly major regression for MonkeyType (which apparently only recently started using LibCST), so might be nice to cut a new release soon

@jimmylai
Copy link
Contributor

Thanks for the quick review! I was wondering what LibCST's release process is like? This feels like a fairly major regression for MonkeyType (which apparently only recently started using LibCST), so might be nice to cut a new release soon

We currently release occasionally and we do prioritize critical bug fixes. There are currently other fixes on master and @josieesh can make a release later this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants