-
Notifications
You must be signed in to change notification settings - Fork 989
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
Format automatically covered lines with flynt #3786
Conversation
Nice! I'm pro this change! The coverage misses are probably genuinely uncovered areas. I think we should write tests for them :) |
So close:
:) |
@balopat ahhh!!! |
@balopat That line might be unreachable and trying to refactor it so that it passes mypy is a time sink for me at the moment. We can merge this and i can look into that line later. |
@balopat Ping for your thoughts. |
This is f-ing awesome. |
lol. regarding my thoughts: I think we can just retry the flynt, and revert the problematic files with no coverage. Then we can open an issue to finalize the flynt conversion on those files as well and fix the coverage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Automerge cancelled: A required status check is not present. Missing statuses: ['cla/google'] |
Automerge cancelled: A required status check is not present. Missing statuses: ['cla/google'] |
1 similar comment
Automerge cancelled: A required status check is not present. Missing statuses: ['cla/google'] |
Should we broadcast this change to cirq-dev? I imagine the new CI behavior will cause more than a few PRs to fail checks. |
Yeah, that's a good idea - I sent out a note! |
It also puts a little promotional message each time
|
Can you open an issue please @mpharrigan? These are valid points and probably could be worked out. |
opened #3901 |
Flynt: A tool to automatically convert old string literal formatting to f-strings
What I did was
This is the first half to #3804