-
Notifications
You must be signed in to change notification settings - Fork 107
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
[Unitary-Hack] Lint rule: Use functions #1579
Conversation
Thanks @Manvi-Agrawal ! Would you mind resolving the conflicts in the branch so I can kick off the checks for this PR? I'll give your questions some more thought once I get the feedback from the unit tests in the CI. |
Done @minestarks. I was on it, and when I came back I saw your comment :-). Also, could we please connect over discord, I have few questions about other open issues xD |
This reverts commit b0b42ff.
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.
A few suggestions to convert some more comments to docstrings.
Co-authored-by: orpuente-MS <[email protected]>
Co-authored-by: orpuente-MS <[email protected]>
Nice catch @minestarks. I realized now that these were originating from unimplemented quick-fix. I had it as TODO, and converting it to no-op(similar to |
Hi @minestarks , @orpuente-MS , I have addressed all review comments. Could you please take a look? |
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.
@Manvi-Agrawal looks good! I will approve.
Thanks a ton @orpuente-MS for your review and approval. |
Hi @minestarks , do you have any other feedback for this PR. Rust panic was really good edge case. Your todays feedback would give me more bandwidth since I can work over that over the weekend, because hack ends on June 12 |
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.
Great work, thanks. Minor cosmetic improvements and then we should be good to go.
Co-authored-by: Mine Starks <[email protected]>
Co-authored-by: Mine Starks <[email protected]>
Just needs one more signoff from @DmitryVasilevsky, @cesarzc, @sezna, @swernli, @tcNickolas (probably due to the small sample update) . |
Fixes #1471.
Future TODO
NeedlessOperation
lint #1593Based on https://github.com/microsoft/qsharp/blob/main/compiler/qsc_passes/src/callable_limits.rs
Note to reviewers
Main changes are in
qsc_linter
folder. Changes in other tests are due to this lint rule complaining about NeedlessOperation, so I converted to function and it changes span accordingly.