From f92c593a3d2c4bdb023fdd834b6e8c874d063cc8 Mon Sep 17 00:00:00 2001 From: Alexander Foremny Date: Thu, 29 Feb 2024 04:11:10 +0100 Subject: feat: add `review` command Prototype of the `review` command, cf. `anissue review -h`. Also adds the `status` command. --- app/Main.hs | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) (limited to 'app/Main.hs') diff --git a/app/Main.hs b/app/Main.hs index 52a316d..5d29923 100644 --- a/app/Main.hs +++ b/app/Main.hs @@ -1,5 +1,5 @@ -- TODO Compute history from the top --- +-- _ -- Currently we are computing the history from the bottom (ie. earliest commit -- first). When computing history from the top, it might allow us to interrupt -- the process and present slightly inaccurate information earlier. @@ -319,6 +319,7 @@ module Main where import Comment qualified as G import Control.Applicative ((<|>)) +import Control.Exception (catch) import Data.Function ((&)) import Data.List (find, intersperse) import Data.Map qualified as M @@ -327,6 +328,7 @@ import Data.Text qualified as T import Data.Text.IO qualified as T import Data.Text.Lazy qualified as LT import Data.Text.Lazy.IO qualified as LT +import Exception qualified as E import Git qualified import History qualified as H import Issue (Issue (..)) @@ -341,7 +343,9 @@ import Options.Applicative qualified as O import Process (proc, sh_, textInput) import Render ((<<<)) import Render qualified as P +import Review qualified as R import Settings (Settings (..), readSettings) +import Status qualified as S import System.Console.Terminal.Size qualified as Terminal import System.Exit (ExitCode (ExitFailure), exitWith) import System.FilePath (()) @@ -424,6 +428,12 @@ data Command | Open { id :: String } + | Status + | Review + { baseBranch :: T.Text, + featureBranch :: T.Text, + granularity :: R.Granularity + } | Search { pattern :: R.RE, closed :: Bool, @@ -444,10 +454,14 @@ cmd = O.progDesc "Show a log of all issues", O.command "open" . O.info openCmd $ O.progDesc "Open file containing an issue", + O.command "review" . O.info reviewCmd $ + O.progDesc "Review changes", O.command "search" . O.info searchCmd $ O.progDesc "List issues matching a pattern", O.command "show" . O.info showCmd $ O.progDesc "Show details of all issues", + O.command "status" . O.info statusCmd $ + O.progDesc "Describe the current anissue action.", O.command "tags" . O.info tagsCmd $ O.progDesc "Show all tags" ] @@ -480,6 +494,36 @@ openCmd = Open <$> idArg +reviewCmd :: O.Parser Command +reviewCmd = + Review + <$> baseBranchArg + <*> featureBranchArg + <*> granularityArg + +baseBranchArg :: O.Parser T.Text +baseBranchArg = + O.option O.auto $ + O.long "base" + <> O.short 'b' + <> O.metavar "BRANCH" + <> O.help "Base branch from which to review changes. Defaults to `main`." + <> O.value "main" + +featureBranchArg :: O.Parser T.Text +featureBranchArg = + O.strArgument (O.metavar "BRANCH_NAME" <> O.value "HEAD") + +granularityArg :: O.Parser R.Granularity +granularityArg = + O.option + O.auto + ( O.long "granularity" + <> O.metavar "GRANULARITY" + <> O.help "Granularity of the review. One of `as-one`, `per-commit`, `per-file` or `per-hunk`. Default: `as-one`." + <> O.value R.AsOne + ) + showCmd :: O.Parser Command showCmd = Show @@ -492,6 +536,10 @@ patternArg = (O.maybeReader R.compileRegex) (O.metavar "PATTERN") +statusCmd :: O.Parser Command +statusCmd = + pure Status + tagsCmd :: O.Parser Command tagsCmd = pure Tags @@ -548,6 +596,20 @@ main :: IO () main = do settings <- readSettings O.execParser (O.info (options <**> O.helper) O.idm) >>= \case + Options {colorize, noPager, width, command = Status} -> do + status <- S.readStatus ".anissue/status" + putDoc colorize noPager width status + Options {colorize, noPager, width, command = Review {baseBranch, featureBranch, granularity}} -> do + sh_ "test -z $(git status --porcelain --untracked-files=no)" + `catch` \(_ :: E.ProcessException) -> + error "working directory not clean, aborting.." + S.withReviewing + (putDoc colorize noPager width) + granularity + baseBranch + featureBranch + ".anissue/status" + S.continueReview Options {colorize, noPager, width, command = List {sort, filters, files, group = Just group, closed}} -> do ungroupedIssues <- I.applySorts sort -- cgit v1.2.3 From a2f401ca9839b6041b7d94f77de4530f168b12ad Mon Sep 17 00:00:00 2001 From: Fabian Kirchner Date: Wed, 6 Mar 2024 13:47:17 +0100 Subject: review(feature/review): approve lgtm, possible splitting some non-business-logic stuff into separate commits would be nice commit 9a49ec0dcd6f75736949350844f85d80fe48a662 --- app/Main.hs | 1 + 1 file changed, 1 insertion(+) (limited to 'app/Main.hs') diff --git a/app/Main.hs b/app/Main.hs index 5d29923..a28f4a6 100644 --- a/app/Main.hs +++ b/app/Main.hs @@ -610,6 +610,7 @@ main = do featureBranch ".anissue/status" S.continueReview +-- REVIEW Why is withReviewing in the Status module and not the Review module? Options {colorize, noPager, width, command = List {sort, filters, files, group = Just group, closed}} -> do ungroupedIssues <- I.applySorts sort -- cgit v1.2.3 From e7450765081e31341496a3f8ac91bda119b55f5a Mon Sep 17 00:00:00 2001 From: Alexander Foremny Date: Wed, 13 Mar 2024 04:36:20 +0100 Subject: chore: drop `--granularity` for `--per-commit` --- app/Main.hs | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) (limited to 'app/Main.hs') diff --git a/app/Main.hs b/app/Main.hs index a28f4a6..634e085 100644 --- a/app/Main.hs +++ b/app/Main.hs @@ -1,5 +1,5 @@ -- TODO Compute history from the top --- _ +-- -- Currently we are computing the history from the bottom (ie. earliest commit -- first). When computing history from the top, it might allow us to interrupt -- the process and present slightly inaccurate information earlier. @@ -343,7 +343,6 @@ import Options.Applicative qualified as O import Process (proc, sh_, textInput) import Render ((<<<)) import Render qualified as P -import Review qualified as R import Settings (Settings (..), readSettings) import Status qualified as S import System.Console.Terminal.Size qualified as Terminal @@ -432,7 +431,7 @@ data Command | Review { baseBranch :: T.Text, featureBranch :: T.Text, - granularity :: R.Granularity + perCommit :: Bool } | Search { pattern :: R.RE, @@ -499,7 +498,7 @@ reviewCmd = Review <$> baseBranchArg <*> featureBranchArg - <*> granularityArg + <*> perCommitArg baseBranchArg :: O.Parser T.Text baseBranchArg = @@ -514,14 +513,11 @@ featureBranchArg :: O.Parser T.Text featureBranchArg = O.strArgument (O.metavar "BRANCH_NAME" <> O.value "HEAD") -granularityArg :: O.Parser R.Granularity -granularityArg = - O.option - O.auto - ( O.long "granularity" - <> O.metavar "GRANULARITY" - <> O.help "Granularity of the review. One of `as-one`, `per-commit`, `per-file` or `per-hunk`. Default: `as-one`." - <> O.value R.AsOne +perCommitArg :: O.Parser Bool +perCommitArg = + O.switch + ( O.long "per-commit" + <> O.help "Review commits individually. (Default: review combined patches)" ) showCmd :: O.Parser Command @@ -599,13 +595,13 @@ main = do Options {colorize, noPager, width, command = Status} -> do status <- S.readStatus ".anissue/status" putDoc colorize noPager width status - Options {colorize, noPager, width, command = Review {baseBranch, featureBranch, granularity}} -> do + Options {colorize, noPager, width, command = Review {baseBranch, featureBranch, perCommit}} -> do sh_ "test -z $(git status --porcelain --untracked-files=no)" `catch` \(_ :: E.ProcessException) -> error "working directory not clean, aborting.." S.withReviewing (putDoc colorize noPager width) - granularity + perCommit baseBranch featureBranch ".anissue/status" -- cgit v1.2.3 From b9f4ee069228e80dda60bc10436693df0aee77ea Mon Sep 17 00:00:00 2001 From: Alexander Foremny Date: Wed, 13 Mar 2024 04:46:13 +0100 Subject: chore: drop `Status` --- app/Main.hs | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) (limited to 'app/Main.hs') diff --git a/app/Main.hs b/app/Main.hs index 634e085..0154840 100644 --- a/app/Main.hs +++ b/app/Main.hs @@ -322,6 +322,7 @@ import Control.Applicative ((<|>)) import Control.Exception (catch) import Data.Function ((&)) import Data.List (find, intersperse) +import Data.List.NonEmpty qualified as NE import Data.Map qualified as M import Data.Maybe (fromMaybe) import Data.Text qualified as T @@ -340,11 +341,12 @@ import Issue.Render () import Issue.Sort qualified as I import Options.Applicative ((<**>)) import Options.Applicative qualified as O +import Patch qualified as A import Process (proc, sh_, textInput) -import Render ((<<<)) +import Render (renderAsText, (<<<)) import Render qualified as P +import Review qualified as R import Settings (Settings (..), readSettings) -import Status qualified as S import System.Console.Terminal.Size qualified as Terminal import System.Exit (ExitCode (ExitFailure), exitWith) import System.FilePath (()) @@ -427,7 +429,6 @@ data Command | Open { id :: String } - | Status | Review { baseBranch :: T.Text, featureBranch :: T.Text, @@ -459,8 +460,6 @@ cmd = O.progDesc "List issues matching a pattern", O.command "show" . O.info showCmd $ O.progDesc "Show details of all issues", - O.command "status" . O.info statusCmd $ - O.progDesc "Describe the current anissue action.", O.command "tags" . O.info tagsCmd $ O.progDesc "Show all tags" ] @@ -532,10 +531,6 @@ patternArg = (O.maybeReader R.compileRegex) (O.metavar "PATTERN") -statusCmd :: O.Parser Command -statusCmd = - pure Status - tagsCmd :: O.Parser Command tagsCmd = pure Tags @@ -592,21 +587,24 @@ main :: IO () main = do settings <- readSettings O.execParser (O.info (options <**> O.helper) O.idm) >>= \case - Options {colorize, noPager, width, command = Status} -> do - status <- S.readStatus ".anissue/status" - putDoc colorize noPager width status - Options {colorize, noPager, width, command = Review {baseBranch, featureBranch, perCommit}} -> do + Options {command = Review {baseBranch, featureBranch, perCommit}} -> do sh_ "test -z $(git status --porcelain --untracked-files=no)" `catch` \(_ :: E.ProcessException) -> error "working directory not clean, aborting.." - S.withReviewing - (putDoc colorize noPager width) - perCommit - baseBranch - featureBranch - ".anissue/status" - S.continueReview --- REVIEW Why is withReviewing in the Status module and not the Review module? + plan <- R.formulatePlan perCommit baseBranch featureBranch + patch <- + A.Patch . concat + <$> mapM + ( \step -> do + R.separateReview step.commit step.changes + =<< R.reviewPatch step.changes + ) + (NE.toList plan.steps) + T.writeFile "review.patch" (renderAsText patch) + -- REVIEW Why is withReviewing in the Status module and not the Review + -- module? + -- + -- RESOLVED `Status` has been dropped in this commit Options {colorize, noPager, width, command = List {sort, filters, files, group = Just group, closed}} -> do ungroupedIssues <- I.applySorts sort -- cgit v1.2.3 From 75444b933f1f23223576fe0ced682b558393ed21 Mon Sep 17 00:00:00 2001 From: Alexander Foremny Date: Wed, 13 Mar 2024 05:27:44 +0100 Subject: chore: patch shows commit messages --- app/Main.hs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) (limited to 'app/Main.hs') diff --git a/app/Main.hs b/app/Main.hs index 0154840..a2fef0b 100644 --- a/app/Main.hs +++ b/app/Main.hs @@ -592,14 +592,7 @@ main = do `catch` \(_ :: E.ProcessException) -> error "working directory not clean, aborting.." plan <- R.formulatePlan perCommit baseBranch featureBranch - patch <- - A.Patch . concat - <$> mapM - ( \step -> do - R.separateReview step.commit step.changes - =<< R.reviewPatch step.changes - ) - (NE.toList plan.steps) + patch <- A.Patch . concat <$> mapM R.reviewStep (NE.toList plan.steps) T.writeFile "review.patch" (renderAsText patch) -- REVIEW Why is withReviewing in the Status module and not the Review -- module? -- cgit v1.2.3 From f63a777c07094af31c8841a3f50af8beca0aa369 Mon Sep 17 00:00:00 2001 From: Alexander Foremny Date: Wed, 13 Mar 2024 05:35:58 +0100 Subject: chore: add review commit template --- app/Main.hs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'app/Main.hs') diff --git a/app/Main.hs b/app/Main.hs index a2fef0b..5a21787 100644 --- a/app/Main.hs +++ b/app/Main.hs @@ -343,7 +343,7 @@ import Options.Applicative ((<**>)) import Options.Applicative qualified as O import Patch qualified as A import Process (proc, sh_, textInput) -import Render (renderAsText, (<<<)) +import Render ((<<<)) import Render qualified as P import Review qualified as R import Settings (Settings (..), readSettings) @@ -592,8 +592,8 @@ main = do `catch` \(_ :: E.ProcessException) -> error "working directory not clean, aborting.." plan <- R.formulatePlan perCommit baseBranch featureBranch - patch <- A.Patch . concat <$> mapM R.reviewStep (NE.toList plan.steps) - T.writeFile "review.patch" (renderAsText patch) + R.commitReview plan . A.Patch . concat + =<< mapM R.reviewStep (NE.toList plan.steps) -- REVIEW Why is withReviewing in the Status module and not the Review -- module? -- -- cgit v1.2.3 From 428e61e202fefe7941a78bd57f5e14d056a74702 Mon Sep 17 00:00:00 2001 From: Fabian Kirchner Date: Wed, 13 Mar 2024 14:22:05 +0100 Subject: fix: parse base branch argument --- app/Main.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/Main.hs') diff --git a/app/Main.hs b/app/Main.hs index 5a21787..bed31c0 100644 --- a/app/Main.hs +++ b/app/Main.hs @@ -501,7 +501,7 @@ reviewCmd = baseBranchArg :: O.Parser T.Text baseBranchArg = - O.option O.auto $ + O.strOption $ O.long "base" <> O.short 'b' <> O.metavar "BRANCH" -- cgit v1.2.3 From f2e36372b4e07993a4992ab29585d4876b28e094 Mon Sep 17 00:00:00 2001 From: Fabian Kirchner Date: Wed, 13 Mar 2024 14:38:13 +0100 Subject: review: approve feature/review Reviewed branch feature/review at commit 07aacbbe55f4793daa0c9893efe4a64951575081. --- app/Main.hs | 1 + 1 file changed, 1 insertion(+) (limited to 'app/Main.hs') diff --git a/app/Main.hs b/app/Main.hs index bed31c0..2729511 100644 --- a/app/Main.hs +++ b/app/Main.hs @@ -516,6 +516,7 @@ perCommitArg :: O.Parser Bool perCommitArg = O.switch ( O.long "per-commit" +-- REVIEW Maybe a short variant would be nice, e.g. '-c'. <> O.help "Review commits individually. (Default: review combined patches)" ) -- cgit v1.2.3 From e0ffe3e85faf4a52fbacf56edc6067c44773a8b1 Mon Sep 17 00:00:00 2001 From: Alexander Foremny Date: Thu, 14 Mar 2024 07:04:09 +0100 Subject: chore: add future review todos --- app/Main.hs | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) (limited to 'app/Main.hs') diff --git a/app/Main.hs b/app/Main.hs index 2729511..d49f627 100644 --- a/app/Main.hs +++ b/app/Main.hs @@ -1,3 +1,39 @@ +-- TODO Add `anissue review-comments` +-- +-- The command `review-comments` should list all review comments within the current review. +-- +-- @assigned aforemny +-- @priority medium +-- @topic review + +-- TODO `anissue review --base` should take own commits into account +-- +-- To facilitate reviewing, the `--base` parameter of `anissue review` should implement some heuristic to review a set of changes multiple times. +-- +-- The first time a review is performed, `--base` should default to the base branch. Any subsequent time, it should default to the last review commit added by myself. +-- +-- @assigned aforemny +-- @priority high +-- @topic review + +-- TODO Add `anissue merge` +-- +-- The command `anissue merge` should merge the currenlty checked out feature request, if there are no unresolved review comments. +-- +-- If there are unresolved review comments, it should fail with a warning. +-- +-- @assigned aforemny +-- @priority high +-- @topic review + +-- TODO Add `anissue request-review` +-- +-- The command `request-review` should create an empty commit, stating that a review is requested. It should mention the eventual "base branch" for inclusion of the feature. +-- +-- @assigned aforemny +-- @priority high +-- @topic review + -- TODO Compute history from the top -- -- Currently we are computing the history from the bottom (ie. earliest commit -- cgit v1.2.3 From 1ed90c7eb2e01dac6604608795d2c0756f1e9f4d Mon Sep 17 00:00:00 2001 From: Alexander Foremny Date: Thu, 14 Mar 2024 07:04:16 +0100 Subject: chore: format Main.hs --- app/Main.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/Main.hs') diff --git a/app/Main.hs b/app/Main.hs index d49f627..3bd8d82 100644 --- a/app/Main.hs +++ b/app/Main.hs @@ -552,7 +552,7 @@ perCommitArg :: O.Parser Bool perCommitArg = O.switch ( O.long "per-commit" --- REVIEW Maybe a short variant would be nice, e.g. '-c'. + -- REVIEW Maybe a short variant would be nice, e.g. '-c'. <> O.help "Review commits individually. (Default: review combined patches)" ) -- cgit v1.2.3 From 8dcace960b25813b00e5f77be25aa7db57851f79 Mon Sep 17 00:00:00 2001 From: Alexander Foremny Date: Thu, 14 Mar 2024 07:06:48 +0100 Subject: review: request-changes feature/review --- app/Main.hs | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'app/Main.hs') diff --git a/app/Main.hs b/app/Main.hs index 3bd8d82..c35d827 100644 --- a/app/Main.hs +++ b/app/Main.hs @@ -553,6 +553,10 @@ perCommitArg = O.switch ( O.long "per-commit" -- REVIEW Maybe a short variant would be nice, e.g. '-c'. + -- + -- REVIEW Sure!, but I don't want to block on this, and I don't think this warrants a tracking issue. + -- + -- RESOLVED <> O.help "Review commits individually. (Default: review combined patches)" ) -- cgit v1.2.3 From 09e26c37de7e7227d856ffe15c9554af36b50c58 Mon Sep 17 00:00:00 2001 From: Alexander Foremny Date: Thu, 14 Mar 2024 07:07:28 +0100 Subject: review: request-changes feature/review --- app/Main.hs | 9 --------- 1 file changed, 9 deletions(-) (limited to 'app/Main.hs') diff --git a/app/Main.hs b/app/Main.hs index c35d827..f9fedea 100644 --- a/app/Main.hs +++ b/app/Main.hs @@ -552,11 +552,6 @@ perCommitArg :: O.Parser Bool perCommitArg = O.switch ( O.long "per-commit" - -- REVIEW Maybe a short variant would be nice, e.g. '-c'. - -- - -- REVIEW Sure!, but I don't want to block on this, and I don't think this warrants a tracking issue. - -- - -- RESOLVED <> O.help "Review commits individually. (Default: review combined patches)" ) @@ -635,10 +630,6 @@ main = do plan <- R.formulatePlan perCommit baseBranch featureBranch R.commitReview plan . A.Patch . concat =<< mapM R.reviewStep (NE.toList plan.steps) - -- REVIEW Why is withReviewing in the Status module and not the Review - -- module? - -- - -- RESOLVED `Status` has been dropped in this commit Options {colorize, noPager, width, command = List {sort, filters, files, group = Just group, closed}} -> do ungroupedIssues <- I.applySorts sort -- cgit v1.2.3