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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Require Absolute paths in branch util combinators from the root branch #3817
base: trunk
Are you sure you want to change the base?
Conversation
@@ -113,24 +112,24 @@ getBranch (p, seg) b = case Path.toList p of | |||
(Branch.head <$> Map.lookup h (Branch._children b)) | |||
>>= getBranch (Path.fromList p, seg) | |||
|
|||
makeAddTermName :: Path.Split -> Referent -> Metadata -> (Path, Branch0 m -> Branch0 m) | |||
makeAddTermName :: (p, NameSegment) -> Referent -> Metadata -> (p, Branch0 m -> Branch0 m) |
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.
These are all now polymorphic on the path type because there are "stepManyAt" combinators in the Cli monad which operate over the root, and need Absolute
, and ones which accept a branch, that use Path
.
I wonder if we should have a more descriptive type name besides |
@aryairani I could add some sort of |
Pinging @aryairani again 馃槃
|
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.
I could add some sort of type GenericSplit p = (p, NameSegment), is that what you have in mind?
Yeah I think that will be slightly better, if you don't mind.
Overview
During #3798 @rlmark and I were working on some stuff and there was some confusion about what sort of
Path
needed to be passed to themakeDelete
combinator; It previously accepts just aPath
. The logic requires that this be relative to the root of the codebase, but it's pretty easy to accidentally pass a Path relative to the current location instead 馃槵 , and it wasn't clear to her as a new contributor which sort of path to pass.This changes all of the MonadUtils combinators to explicitly accept an Absolute split so it's clear where the path must be rooted.
As you'll see in the diff, in most spots we already had an absolute path anyways and were just converting before passing it it, might as well convert it after passing it in and make things a bit clearer and more type-safe.
Implementation notes
make*
combinators in BranchUtil to accept and return any flavour of Path in their splits. This is polymorphic over the path type because if you're usingBranch.stepMany
rather thanCli.stepMany
you'll need a Path, since it may not be relative to the root.Cli.stepMany
-style combinators, since these are all going to be rooted at the root of the codebase.Test coverage
Existing transcripts should be sufficient.