Move file content stuff into a filec module in Fs #20
Conversation
chshersh
left a comment
chshersh
left a comment
There was a problem hiding this comment.
Awesome refactoring!
I like how different parts are now more separated, and we properly create modules with clear responsibilities.
I left a few minor comments about the naming and consistency of the interface but overall it looks great!
lib/fs/filec.mli
Outdated
| (** Reads file contents using 'bat' to have pretty syntax highlighting **) | ||
| val read_file_contents : string -> t | ||
|
|
||
| (** Returns offset based on file type of contents **) | ||
| val offset_from_file_contents : t -> int | ||
|
|
||
| (** Returns the len of file contents **) | ||
| val line_len_from_file_contents : t -> int | ||
|
|
||
| (** Returns the lines of file contents **) | ||
| val lines_from_file_contents : t -> Pretty.doc array |
There was a problem hiding this comment.
Let's simplify names of the functions here. Since they're prefixed with Filec, there's no need to add the file_contents suffix.
This is a typical trick in OCaml to use modules as kinda namespaces. So these functions will become:
Filec.read
Filec.offset
Filec.length
Filec.lines
lib/fs/filec.ml
Outdated
|
|
||
| let lines_from_file_contents = function | ||
| | Text { lines; _ } -> lines | ||
| | Binary -> [| Pretty.str binary_file_warning |] |
There was a problem hiding this comment.
Let's do a minor performance optimisation:
Move [| Pretty.str binary_file_warning |] into a global variable and just return it here.
Generally, this is not safe because Arrays are mutable in OCaml. But we don't change this array, so I feel like it should be fine.
lib/fs/filec.ml
Outdated
| let line_len_from_file_contents = function | ||
| | Text { lines; _ } -> Array.length lines | ||
| | Binary -> 1 |
There was a problem hiding this comment.
I would simplify this function by calling Filec.lines first and then Array.length on the result.
This way the Binary case will be automatically consistent with how many lines are inside and the interface will be less error-prone.
Good idea! There also could be some standard module |
chshersh
added
type: refactoring
|
I think I addressed the suggestions you pointed out, lmk what you think! |
chshersh
left a comment
chshersh
left a comment
There was a problem hiding this comment.
Great stuff!
I left only one minor suggestion but overall it looks great!
Co-authored-by: Dmitrii Kovanikov <[email protected]>
heathhenley
force-pushed
the
dev_lunchtime_caml_practice
branch
from
1da7dd1 to
1c5ac0e
Compare
Co-authored-by: Dmitrii Kovanikov <[email protected]>
|
Rebased and applied the latest suggestion ✅ |
2 participants
An attempt at addressing: #16
I used the name
Filecto avoid any conflict with theFileconstructor for thetreetype inFshere: https://github.com/chshersh/github-tui/blob/main/lib/fs/fs.mli#L12 (eg so that there's no confusion matching onFs.File/Fs.Dirvs usingFs.File.tand the functions in there)