Refactor file tree type #52
Conversation
chshersh
left a comment
chshersh
left a comment
There was a problem hiding this comment.
Great stuff!
I left a few minor comments. Since we're improving the quality of the project, I proposed to make a few more tiny changes 😅
| | File { name; _ } -> name | ||
| | Dir { name; _ } -> name |
There was a problem hiding this comment.
This was actually the main motivation behind doing this minor refactoring.
The internal contents of constructors like File and Dir might change by adding and removing some fields (I already expect a few possible changes). But these changes will be decoupled from functions that don't affect them, so it'll be easier to perform them.
Even such minor things matter when a project grows.
| let rec sort_tree = function | ||
| | File (name, contents, ft) -> File (name, contents, ft) | ||
| | Dir (name, (lazy children)) -> | ||
| | File _ as f -> f |
lib/fs/fs.ml
Outdated
| in | ||
| let dirname = Filename.basename path in | ||
| Dir (dirname, children) | ||
| Dir { name = dirname; children } |
There was a problem hiding this comment.
Let's just rename dirname above to name to leverage punning and write just Dir { name; children }.
There was a problem hiding this comment.
In fact, let's do one more step and move this new name before if.
This way we can reuse it in both Dir and File cases.
| and current offsets. *) | ||
| module Filec = Filec | ||
|
|
||
| (** A definition of a file tree. *) |
There was a problem hiding this comment.
One minor addition while we're on this: let's improve docs by adding a note saying something like that contents and file_type are stored separately as two lazy values so we can quickly understand the file type and use a proper icon without reading the whole contents.
Just a note for future selves so we don't forget the motivation 😅
lib/tui/tui.ml
Outdated
| match tree with | ||
| | Fs.File (path, _, _) -> | ||
| | Fs.File { name = path; _ } -> | ||
| Printf.eprintf "Given path '%s' is not a directory!" path; |
There was a problem hiding this comment.
I'm actually fine if you don't use name = path and just use name. Makes the code consistent with others.
I now think that path might not be the most accurate variable here.
chshersh
added
type: refactoring
…factor_file_tree_type
2 participants
closes #35