-
Notifications
You must be signed in to change notification settings - Fork 31
Fix a typo in the TOperator
test
#572
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
Conversation
clang-tidy review says "All clean, LGTM! 👍" |
I cannot see a codecov bot comment. Is it not running? |
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.
The change being done here is the self
argument. Using both object
and toperator
is acceptable. And in both cases we are reusing the variable declared before.
I would say object
is better as it conveys the correct usage.
clang-tidy review says "All clean, LGTM! 👍" |
Could you elaborate on the correctness of using |
@Vipul-Cariappa This is due to the accidentally removal of the coverage job from the ci. This PR fixes it #577 . Once this PR is in, rebase this PR and you should see a coverage comment. Feel free to review and approval the PR, if it hasn't already been done, when you see this message. |
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.
My bad. I assumed you were invoking a constructor, so I thought it did not matter what the object pointed to. But I see that it is a method that is being invoked. This is correct, and the previous usage was wrong.
LGTM!
We may need to find a way to catch this.