From 2218c28618fc918998fdb5aeb7f89f3105e918bb Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 31 Aug 2017 19:03:12 -0400 Subject: review --- ...ent_1_74c6576b25f74c6e620eb015af8b0f6a._comment | 26 ++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_1_74c6576b25f74c6e620eb015af8b0f6a._comment (limited to 'doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_1_74c6576b25f74c6e620eb015af8b0f6a._comment') diff --git a/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_1_74c6576b25f74c6e620eb015af8b0f6a._comment b/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_1_74c6576b25f74c6e620eb015af8b0f6a._comment new file mode 100644 index 00000000..5982361f --- /dev/null +++ b/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_1_74c6576b25f74c6e620eb015af8b0f6a._comment @@ -0,0 +1,26 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 1""" + date="2017-08-31T22:40:34Z" + content=""" +That's a pretty nice job for your first haskell code! And an impressive +module. + +Most of my review comments have to do with improving types.. Which is +always a nice way to improve already good code. :) + +* VolumeGroup and LogicalVolume seem like easy things to mix up. + Also, there's never a LogicalVolume without an associated VolumeGroup. + So, suggest `newtype VolumeGroup = VolumeGroup String` and + `data LogicalVolume = LogicalVolume String VolumeGroup` -- then + the user would write something like + `LogicalVolume "test" (VolumeGroup "vg0")` +* Why not make `LvState` contain a `Maybe Partition.Fs` rather than + the string value. (This also would move the parsing of filesystem names + from `fsMatch` to `lvState` or perhaps to another function it uses.) +* It seems a bit wrong for `parseSize` to include the rounding + to the next extent, which is not really related to parsing. + Would be better to split those two things into separate functions. + +I feel that this module is fairly close to mergeable. +"""]] -- cgit v1.2.3