Reply on merge command parse failure #103

rlycx · 2022-03-11T15:57:37Z

Makes hoff give feedback when an invalid command is issued.

bertptrs

Implementation looks alright to me but I also don't really speak Haskell. I'll tag a second reviewer.

joris-burgers

I suspect this will never leave a comment that the command was not recognized.

It can also be found by adding a test, like this one:

hoff/tests/Spec.hs

Lines 829 to 842 in 7f0235c

    
           it "doesn't allow 'merge and tag' command on Friday" $ do 
        
             let 
        
               prId = PullRequestId 1 
        
               state = singlePullRequestState prId (Branch "p") masterBranch (Sha "abc1234") "tyrell" 
        
               event = CommentAdded prId "deckard" "@bot merge and tag" 
        
               results = defaultResults { resultIntegrate = [Right (Sha "def2345")], resultGetDateTime = repeat (T.UTCTime (T.fromMondayStartWeek 2021 2 5) (T.secondsToDiffTime 0)) } 
        
               (_, actions) = runActionCustom results $ handleEventTest event state 
        
             actions `shouldBe` 
        
               [ AIsReviewer "deckard" 
        
               , ALeaveComment prId "Your merge request has been denied, because merging on Fridays is not recommended. To override this behaviour use the command `merge and tag on Friday`." 
        
               ]

src/Logic.hs

joris-burgers

Looks a lot better, I left some smaller questions.

src/Logic.hs

joris-burgers · 2022-03-14T10:32:15Z

src/Logic.hs

+data ParseResult a
+  = Success a
+  | Unknown Text
+  | Ignored


Nice datatype, can you comment it a bit more, as well as the isSuccess function.

joris-burgers · 2022-03-14T10:35:23Z

src/Logic.hs

+        -- The bot was mentioned but encountered an invalid command, report error and
+        -- take not further action
+        Unknown command -> do
+          () <- leaveComment prId ("`" <> command <> "` was not recognized as a valid command.")


Not sure, but could this trigger a comment loop? Say I comment @bot megre, the comment will be included in the response and the @bot would trigger Hoff again. Could that happen or are we protected against that? Otherwise we can just reply that the last comment was not recognized, people will see what they typed.

I think it should be alright because it uses Text.isPrefixOf, but just to be safe, I'll strip the prefix from command before putting it in the comment.

joris-burgers · 2022-03-14T10:36:09Z

src/Logic.hs

+          () <- leaveComment prId ("`" <> command <> "` was not recognized as a valid command.")
+          pure state
+        -- Cases where the parse was successful AND the author is a reviewer
+        Success command | isApproved -> case command of


Personally, I find this guard a bit subtle for the security check. Could you make it an explicit if, that way changing the order of the guards doesn't become a security concern.

Alternatively, you could attach both guards to the same branch, i.e.

case .. of ... Success command | isApproved -> ... | otherwise -> ...

which IMO is the nicer style even in the absence of security concerns, as it makes it very clear which sub-cases there are for the Success case.

Also, isApproved seems to be the conjunction of the success case and the author-is-reviewer case. But you're already checking the success case by explicit pattern matching now. So just checking for isReviewer in the guard would be even cleaner.

Thanks, I think I'll adopt the style mentioned by @fatho :).

Regarding isReviewer, putting it in the guard makes sense, but the function returns Action, so it can't be done within the guard itself (as far as I know), so doing isSuccess and also pattern matching on the Success case is a bit of a compromise to keep the bot from performing an Action every time it reads a comment regardless of whether it parsed a command. :/

…heck

joris-burgers

Some minor style points, but otherwise LGTM.

src/Logic.hs

joris-burgers

One more thing, otherwise LGTM.

joris-burgers · 2022-03-14T16:25:53Z

src/Logic.hs

+-- | consumer of `parseMergeCommand` to inspect the reason why a message
+-- | was considered invalid.


These pipes should only go in front on the first comment. See https://haskell-haddock.readthedocs.io/en/latest/markup.html.

Thanks! Fixed :)

joris-burgers

LGTM!

rlycx · 2022-03-14T16:42:03Z

Thanks for all the feedback! :)

rlycx · 2022-03-16T07:56:16Z

@OpsBotPrime merge

OpsBotPrime · 2022-03-16T07:56:17Z

Pull request approved for merge by @RileyApeldoorn, rebasing now.

Approved-by: RileyApeldoorn Auto-deploy: false

OpsBotPrime · 2022-03-16T07:56:22Z

Rebased as 0bd569a, waiting for CI …

Reply on merge command parse failure

b516427

rlycx linked an issue Mar 11, 2022 that may be closed by this pull request

Reply on unknown commands #102

Closed

rlycx requested a review from bertptrs March 14, 2022 08:26

bertptrs approved these changes Mar 14, 2022

View reviewed changes

bertptrs requested a review from joris-burgers March 14, 2022 08:29

joris-burgers requested changes Mar 14, 2022

View reviewed changes

src/Logic.hs Outdated Show resolved Hide resolved

Add test case and nicer message that specifies what was not recognized

1d70a37

rlycx requested a review from joris-burgers March 14, 2022 10:25

Accidentally left an empty line

e8d58a3

joris-burgers reviewed Mar 14, 2022

View reviewed changes

Riley Apeldoorn added 2 commits March 14, 2022 12:34

Document ParseResult and isSuccess, improve readability of reviewer c…

f564d7f

…heck

Strip bot self-mention from unknown command comment just in case

531a64a

rlycx requested a review from joris-burgers March 14, 2022 14:44

joris-burgers reviewed Mar 14, 2022

View reviewed changes

src/Logic.hs Outdated Show resolved Hide resolved

src/Logic.hs Outdated Show resolved Hide resolved

Fix style, prevent esoteric error message

a64c232

rlycx requested a review from joris-burgers March 14, 2022 15:30

joris-burgers reviewed Mar 14, 2022

View reviewed changes

Fix doc comment

3a4168f

rlycx requested a review from joris-burgers March 14, 2022 16:29

joris-burgers approved these changes Mar 14, 2022

View reviewed changes

Merge #103: Reply on merge command parse failure

0bd569a

Approved-by: RileyApeldoorn Auto-deploy: false

OpsBotPrime merged commit 0bd569a into master Mar 16, 2022

OpsBotPrime deleted the report-invalid-command branch March 16, 2022 08:04

	it "doesn't allow 'merge and tag' command on Friday" $ do
	let
	prId = PullRequestId 1
	state = singlePullRequestState prId (Branch "p") masterBranch (Sha "abc1234") "tyrell"

	event = CommentAdded prId "deckard" "@bot merge and tag"

	results = defaultResults { resultIntegrate = [Right (Sha "def2345")], resultGetDateTime = repeat (T.UTCTime (T.fromMondayStartWeek 2021 2 5) (T.secondsToDiffTime 0)) }
	(_, actions) = runActionCustom results $ handleEventTest event state

	actions `shouldBe`
	[ AIsReviewer "deckard"
	, ALeaveComment prId "Your merge request has been denied, because merging on Fridays is not recommended. To override this behaviour use the command `merge and tag on Friday`."
	]

		-- \| consumer of `parseMergeCommand` to inspect the reason why a message
		-- \| was considered invalid.

Reply on merge command parse failure #103

Reply on merge command parse failure #103

Uh oh!

Conversation

rlycx commented Mar 11, 2022

Uh oh!

bertptrs left a comment

Choose a reason for hiding this comment

Uh oh!

joris-burgers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joris-burgers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joris-burgers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joris-burgers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joris-burgers left a comment

Choose a reason for hiding this comment

Uh oh!

rlycx commented Mar 14, 2022

Uh oh!

rlycx commented Mar 16, 2022

Uh oh!

OpsBotPrime commented Mar 16, 2022

Uh oh!

OpsBotPrime commented Mar 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants