Lazify dir reading #19
Conversation
heathhenley
force-pushed
the
dev_lazify
branch
from
95a8bf1 to
623d265
Compare
chshersh
left a comment
chshersh
left a comment
There was a problem hiding this comment.
Great job!
I left a few minor comments about using the lazy pattern to slightly simplify some code.
But overall, I like how this refactoring was straightforward 🏆
lib/fs/fs.ml
Outdated
| | Dir (name, children) -> | ||
| let children = Lazy.force children in |
There was a problem hiding this comment.
Let's use the lazy pattern in the pattern matching itself instead of Lazy.force. The behavior will be the same but the code will be slightly terser:
| Dir (name, lazy children) ->There was a problem hiding this comment.
awesome I didn't realize you could do that!
lib/fs/fs.ml
Outdated
| current = File_cursor (Lazy.force contents); | ||
| } | ||
| | Dir (_, next) -> | ||
| let next = Lazy.force next in |
There was a problem hiding this comment.
Same comment about the lazy pattern
| | Dir_selected { children; prev_total; pos } -> ( | ||
| let children = Lazy.force children in | ||
| match children with | ||
| (* No children of a directory without children *) | ||
| | [||] -> Empty_directory | ||
| (* Non-empty array of children *) | ||
| | _ -> Directory_contents (children_to_doc ~prev_total ~pos children)) |
There was a problem hiding this comment.
I think here it's fine to keep Lazy.force because there's no elegant way to pattern-match on a lazy pattern inside record.
chshersh
added
type: refactoring
|
@heathhenley CI was failing due to some bug in the I updated the CI, so if you update branches, the CI should start working again. |
heathhenley
force-pushed
the
dev_lazify
branch
from
623d265 to
611cdaf
Compare
Co-authored-by: Dmitrii Kovanikov <[email protected]>
|
I think I addressed them all - let me know what you think! |
2 participants
An attempt to address #5
Switched type of tree so that that dir is a Lazy.t and added Lazy.force where needed.