BC Features: A few additional features required by other components #12
Conversation
lib/linux_admin/fstab.rb
Outdated
There was a problem hiding this comment.
I'm not a fan of the abbreviated names. One of the goals of LinuxAdmin was to give normal names to things that would not be obvious. For example, mntops should be mount_options (unless I'm wrong on that abbreviation).
There was a problem hiding this comment.
Also, the fs_ is redundant.
Since I've already given one example, my principle-of-least-surprise mind would expect the other accessors to be called:
fs_spec -> spec (maybe specification ?)
fs_file -> file
fs_vfstype -> virtual_fs_type (maybe virtual_type or vfs_type ?)
fs_mntopts -> mount_options
fs_freq -> frequency
fs_passno -> pass_number (maybe pass_num or pass ? ...I have no idea what this actually is)
There was a problem hiding this comment.
I got the names from the fstab man page:
http://linux.die.net/man/5/fstab
See the description of the fields there. Figured it'd be best to keep our adapter in line w/ the official documentation name wise but its not a huge deal either way.
There was a problem hiding this comment.
@movitto @Fryguy I'm ok with using the spec names, but we can always alias them with human readable names and use those names when presenting that information to the user. Maybe we wait until we actually have to present them to the user and then we come up with useful names for them and decide how we alias them, etc.?
There was a problem hiding this comment.
I prefer the human readable names, and we've already started going down the human readable path for other modules within this gem. I don't want to have consult documentation every time I need to understand something. The code should just tell me. Now, for experts who know the values by heart, I could see aliasing the other, more cryptic, names, but for code readability and casual/medium code usage, the friendlier names are better.
EDIT: cryptic is probably the wrong word. The fs_* names are probably the names in C, which makes sense why they are prefix with fs in order to vaoid name collisions. However, we are creating an abstraction, and a hopefully clearer abstraction in Ruby than in C, hence why I'm adamant about friendly names for the attributes.
There was a problem hiding this comment.
After reading the man page, you can see how unclear the names actually are...
My new suggestions:
fs_spec -> block_special_device (or maybe just block_device or device)
fs_file -> mount_point
fs_vfstype -> fs_type (or filesystem_type, but I think fs is a clear abbreviation for an otherwise very long word)
fs_mntopts -> mount_options
fs_freq -> needs_dump (or dumpable?)
fs_passno -> fsck_order
|
Aside from my comment, this looks good. Out of curiosity what was the reason behind returning self in the service methods? Was it so you could chain operations? Should other operations in other classes be changed to return self as well (no in this PR, just asking in general)? |
lib/linux_admin/fstab.rb
Outdated
There was a problem hiding this comment.
I think load_fstab should be made private, as I wouldn't expect callers to call it.
|
I lied...I had 2 more comments :) |
|
Yes on returning self so methods can be chained. Renamed the 'load_fstab' method to 'refresh' and marked it as private. We are going to need to add functionality to remove/add fstab entries (eg remove the pg disk in the black constrol restore operation) so we may want to make this public at some point to allow manual refreshing. Alternatively we could get fancy use inotify to automatically refresh the instance whenever the /etc/fstab file changes but we may not need something that complicated. Can figure it out going forward. I changed !=~ to !~. The former is syntactically valid and didn't throw off the tests, but as you said the later is the correct operator. |
|
@Fryguy Looks good to me. :+1 |
BC Features: A few additional features required by other components
|
Also looks good to me. |
4 participants
No description provided.