-
Notifications
You must be signed in to change notification settings - Fork 267
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
remove FileCodebase and some weeds #2363
Conversation
- moved everything that depended on V1 `Reference` into `FileCodebase` namespace, duplicating a lot of code for better or worse. - pulled these into their own modules: - Codebase (types) into `Codebase.Type`; `Codebase` module re-exports them - `Codebase.Branch.Merge`, - `Codebase.Branch.Names` (Branch shouldn't depend on `Names`) - `Codebase.BuiltinAnnotation` - `Codebase.Causal.FoldHistory` - `Codebase.CodeLookup.Util` (remove `CodeLookup` dependency on `UnisonFile`) - `Codebase.DataDeclaration.ConstructorId` trying to use this alias in relevant places - `Codebase.Init` - `CodebasePath`, the `FilePath` alias. - `Lexer.Pos` (because `AnnotatedText`, `Range`, `Parser.Ann` shouldn't depend on the whole lexer) - `Names.ResolutionResult` (`Type` shouldn't depend on `Names`) - `PrettyPrintEnv.FQN` - `PrettyPrintEnv.Names` (pulled out references to `Names` from `PPE`) - `PPE.Util` not 100% sure what's happening here - `PrettyPrintEnvDecl` pull this data type and supporting functions into separate module - `Path.Parse` (`Path` shouldn't depend on the lexer) - `Path.Convert`, `Path.Parse` into `Unison.Util.Convert` - `Parser.Ann` (`Codebase`, etc. etc. shouldn't depend on `Parser`) - `Referent'` (`SyntaxText`/`ColorText` shouldn't depend on `Reference`) - `Referent` module re-exports stuff from `Referent'`, hard-coded to `Reference`. - removed `SyntaxText.SyntaxText` - `TermEdit.Typing` (because `TermEdit` shouldn't depend on the full typechecker) - `UnisonFile` / `TypecheckedUnisonFile` (types into `UnisonFile.Type`) - `UnisonFile.Env` - `UnisonFile.Error` - `UnisonFile.Names` - `Var.WatchKind` - `Var.refNamed` - Deleted unused `Codebase.Classes` typeclasses wip - Deleted unused `Unison.Util.Menu`, ancient modal stuff - Moved `Codebase.makeSelfContained` into `UnisonFile` module, since it deals with `UnisonFile` and not with `Codebase`. - split up `GitError` into a more codebase-agnostic hierarchy (see `Codebase.Type.GitError`) - changed `bindNames` to `bindReferences` in some cases; `bindNames` remains in `.Names` compatibility module - move `Unison.Var.refNamed` into `Unison.Term` - tweaked GitError to separate obviously codebase-format specific errors from non-obviously-corbase-format-specific errors. - tweaked `Reflog.Entry` to support anything that's coercible to `Unison.Hash`, but also changed its kind - removed `DebugBranchHistoryI` input because I didn't want to maintain its implementation - removed `ShortBranchHash` dependency on `Hash`, and let it work on anything coercible to `Hash`. P.S./todo This class should be called ShortHash; the existing ShortHash is really a ShortReference! - removed SyntaxText dependency on Reference - cleaned up `Path` somewhat - held off on: - move `DD.updateDependencies` - splitting up `Path` into the billion different components - e.g. `Path` becomes `RelativePath`, `Path'` becomes `Path`, and `Absolute` wraps `RelativePath` - `Split` / `Split'` maybe rename to `Path.NonEmpty`? - rename `Branch.getPatch` / `.getMaybePatch` - split out `Name.Parse`, `Name.Convert`, substitute a lot of specific functions like `Path.hqSplitFromName'` with `Convert.parse`. - todo: - clear unreferenced junk - restore: - `NameEdit`? - `PatternCompat`?
and DebugBranchHistoryI
# Conflicts: # parser-typechecker/src/Unison/Codebase.hs # parser-typechecker/src/Unison/Codebase/Editor/HandleCommand.hs # parser-typechecker/src/Unison/PrettyPrintEnv.hs # parser-typechecker/unison/Main.hs # unison-core/src/Unison/Names3.hs # unison-core/src/Unison/Pattern.hs # unison-core/src/Unison/Term.hs # unison-core/src/Unison/Type.hs # unison-core/src/Unison/Util/Relation.hs # unison-core/unison-core1.cabal
and update messages
486ec00
to
81a32fc
Compare
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.
Cool. Left a couple minor questions.
Can you work with @hojberg and @rlmark on refreshing #2365 which I'm guessing will have some conflicts with Main
I think it would be good to document conventions for how modules get broken up, and then start doing that consistently, particularly for new code or when you're touching existing code.
Like if the convention is: Foo
has the type and instances, and Foo.Util
has utility functions (and re-exports Foo
?) then do that consistently. If the convention is that Foo.Type
has the type and instances, and Foo
has the utility functions (and re-exports Foo.Type
?) then do that consistently.
Having a "1 type per module, unless it's a util module that's about re-exporting" guideline might be a good thing to start doing when writing code in the first place.
Possibly relevant: https://www.haskellforall.com/2021/05/module-organization-guidelines-for.html
Yeah, I’ll write something up, to answer these questions, and maybe tweak what’s here to match.
Yeah.
Cool, I’ll check it out. |
Oops, this should have gone with #2363
In addition to removing v1
FileCodebase
support, this PR decouples many module dependencies that I felt were making refactors difficult.Apart from a couple of tweaked output messages, there shouldn't be any functionality changes here.
This will merge-conflict pretty hard with #2365 unfortunately, but I'm happy to help with reconciling them.
Things the PR did
Codebase.Type
;Codebase
module re-exports themCodebasePath
Codebase.Branch.Merge
,Codebase.Branch.Names
(Branch shouldn't depend onNames
)Codebase.BuiltinAnnotation
Codebase.Causal.FoldHistory
Codebase.CodeLookup.Util
(removeCodeLookup
dependency onUnisonFile
)Codebase.DataDeclaration.ConstructorId
trying to use this alias in relevant placesCodebase.Init
CodebasePath
, theFilePath
alias.Lexer.Pos
(becauseAnnotatedText
,Range
,Parser.Ann
shouldn't depend on the whole lexer)Names.ResolutionResult
(Type
shouldn't depend onNames
)PrettyPrintEnv.FQN
(containselideFQN
logic)PrettyPrintEnv.Names
(pulled out references toNames
fromPPE
)PPE.Util
not 100% sure what's happening herePrettyPrintEnvDecl
pull this data type and supporting functions into separate modulePath.Parse
(Path
shouldn't depend on the lexer)Path.Convert
,Path.Parse
intoUnison.Util.Convert
Parser.Ann
(Codebase
, etc. etc. shouldn't depend onParser
)Reference.Util
(Reference
shouldn't depend onABT
)Referent'
(SyntaxText
/ColorText
shouldn't depend onReference
)Referent
module re-exports stuff fromReferent'
, hard-coded toReference
.SyntaxText.SyntaxText
TermEdit.Typing
(becauseTermEdit
shouldn't depend on the full typechecker)UnisonFile
/TypecheckedUnisonFile
(types intoUnisonFile.Type
)UnisonFile.Env
UnisonFile.Error
UnisonFile.Names
Var.WatchKind
Var.refNamed
Codebase.Classes
typeclasses wipUnison.Util.Menu
, ancient modal stuffRuntime.needsContainment
field and related logic.bindNames
tobindReferences
in some cases;bindNames
remains in.Names
compatibility moduleUnison.Var.refNamed
intoUnison.Term
Codebase.Type.GitError
)Reflog.Entry
to support anything that's coercible toUnison.Hash
, but also changed its kindDebugBranchHistoryI
input because I didn't want to maintain its implementationShortBranchHash
dependency onHash
, and let it work on anything coercible toHash
. P.S./todo This class should be called ShortHash; the existing ShortHash is really a ShortReference! (because it contains extras to do with cycles, etc.)Path
somewhatThings I debated
Codebase
,Codebase.Init
,UnisonFile
) where I moved the actual type out of the file, I wanted to separate the definition from the various util functions. I'd be happy to move the util functions instead.data CodebaseFormat = V2
in Main/GetArgs? (like in Test.Ucm) or leave it deleted until we have multiple formats?Things I held off on / maybe later
DD.updateDependencies
Path
into the billion different componentsPath
becomesRelativePath
,Path'
becomesPath
, andAbsolute
wrapsRelativePath
Split
/Split'
maybe rename toPath.NonEmpty
?Branch.getPatch
/.getMaybePatch
Name.Parse
,Name.Convert
type classes, substitute a lot of specific functions likePath.hqSplitFromName'
withConvert.parse
.ShortHash
toShortReference
, andShortBranchHash
toShortHash