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

Add option to edit LGTM message and to turn it off #48

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

bwrsandman
Copy link
Contributor

@bwrsandman bwrsandman commented Jul 31, 2022

Implements #47

  • Add description about empty string skipping LGTM message
  • Test empty message
  • Test Custom message
  • Test with default message

@ZedThree
Copy link
Owner

ZedThree commented Aug 1, 2022

Thanks @bwrsandman! I take it setting it to the empty string means the comment doesn't get posted -- does it need an explicit check, or does GitHub automatically reject empty comments?

@bwrsandman
Copy link
Contributor Author

bwrsandman commented Aug 1, 2022

There is an explicit check here:
https://github.com/ZedThree/clang-tidy-review/pull/48/files#diff-32e111aa752f95e484818ff16dedaa5bf4a768785886e9fa705b2b41764be060R94

I don't know what Github would do with an empty message

I'm currently testing it it works here:
openblack/openblack#526
openblack/openblack#527

@ZedThree
Copy link
Owner

ZedThree commented Aug 1, 2022

Ah, I missed that, sorry! Hadn't had enough caffeine :)

Would you mind just noting in the description that setting it to the empty string turns off the comment?

If you're happy this works for your case, I'll merge it

@bwrsandman
Copy link
Contributor Author

Not sure why but it posts empty quotes...
openblack/openblack#527 (comment)

@ZedThree
Copy link
Owner

ZedThree commented Aug 1, 2022

Because we're passing the arguments through Github's YAML, then Docker, then bash, it's quite possible it's getting passed through as "''". I had something like that before, so there's a strip_enclosing_quotes function that makes sure all the quotes are removed. Then we should end up with an actual empty string and the not body conditional should trigger.

@bwrsandman bwrsandman marked this pull request as draft August 1, 2022 15:52
@bwrsandman bwrsandman marked this pull request as ready for review August 2, 2022 03:15
@bwrsandman
Copy link
Contributor Author

I've tested and added the part about disabling the comment.

Copy link
Owner

@ZedThree ZedThree left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @bwrsandman !

@ZedThree ZedThree merged commit 526cbfb into ZedThree:master Aug 2, 2022
@bwrsandman bwrsandman deleted the editable-lgtm-message branch August 2, 2022 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants