Preserve comments and format generated whitespace in fstab #61
Conversation
|
@movitto How does this pull handle empty lines or whitespace only lines? The motivation to this change was to preserve comments and any empty lines so we can possibly wrap changes to the fstab in comments similar to how other tools do and at the same time not lose any comments/whitesapce that other tools added. |
|
@brandondunne Thoughts? |
|
@jrafanie comment lines are preserved, will look into preserving comments on lines w/ content and empty lines. Am fine w/ sharing / reusing code, less to maintain. |
|
As discussed via email, deferring this for a bit and looking into http://augeas.net/ |
|
@movitto @brandondunne augeas feels like the wrong tool for the job. It has ruby bindings but why do we need a c extension for something we will may use so infrequently and thus who cares if we use slow pure ruby... It might be worthwhile implementing a basic pure ruby code for each file type as we need them. |
|
@jrafanie I feel the case for using it would be it would take care of all the details of parsing and writing the files in question. We'd just have to call out to augeas whenever we'd want to read or write to /etc/fstab or /etc/hosts and wouldn't have to worry about the specifics of those file formats. Agree that adding a platform specific dependency would be a major drawback to this though. Suppose it comes down to how much we could leverage this for. For the time being since we're only doing simple modifications to these two files, agree it probably would make sense to roll our own, but if we need more and/or more complicated config file parsing it would make sense to revisit In any case will look into the remaining issues and update shortly. |
|
@jrafanie @brandondunne updated |
lib/linux_admin/fstab.rb
Outdated
There was a problem hiding this comment.
I don't understand the need for a line_number or even the columns_lengths or the why we need to separate out the comments from entries.
Maybe I'm missing something.
I believe @entries could contain all lines
- regular ones having a device - these are treated as they always have on read and write
- ones with a comment
- blank ones (ones without a device or comment)
We can then read them and store them in @entries and do the right thing with setting the entry attributes by only setting the relevant attributes, such as comments only having the comment attribute, device ones having the other attributes set, and blank lines having nothing set.
Then, when we want to write them, we just loop through @entries one line at a time, writing normal entries as we do now, writing entries with a comment as the comment, and ones without a command or a device as an empty line.
Thoughts?
|
@jrafanie updated to incorporate your feedback. Took a little while to tidy up the code while handling all the edge cases but believe I've got them all (as verified by the specs). |
lib/linux_admin/fstab.rb
Outdated
There was a problem hiding this comment.
I think we can use unless comment.blank?. ActiveSupport is available in this gem.
|
@movitto I had a few style comments, but otherwise this looks great! |
lib/linux_admin/fstab.rb
Outdated
There was a problem hiding this comment.
This column_lengths local variable is confusing because we have a reader method column_lengths and a instance variable column_lengths.
|
@jrafanie updated |
lib/linux_admin/fstab.rb
Outdated
There was a problem hiding this comment.
Consider making this formatting string a method since it wasn't obvious what this was doing at first.
def write!
...
content << entry.columns.map.with_index do |column_value, i|
right_pad(column_value, @column_lengths[i])
...
end
def right_pad(column_value, desired_length)
"%-#{desired_length}s" % column_value
endThere was a problem hiding this comment.
Maybe @columns_lengths could instead be @maximum_column_lengths or something similar to make this more clear?
There was a problem hiding this comment.
or, preferably use the Ruby methods .ljust and .rjust instead of the sprintf format style.
lib/linux_admin/fstab.rb
Outdated
|
I have only minor final comments. I like how you split up the logic into small methods but I believe there are few more things we can do to make it a bit more understandable, especially around the string padding we do in #write! and the calculating the maximum length of each column for the file on read (refresh) to enable the padding (at #write!). |
|
@Fryguy looks good to me. Did you want to review the string padding in formatted_columns? |
lib/linux_admin/fstab.rb
Outdated
There was a problem hiding this comment.
Indent is off here (3 spaces instead of 2). Also needs a space after the comma. Code-wise this looks good though.
|
Looks good to me. @jrafanie? |
Preserve comments and format generated whitespace in fstab
5 participants
No description provided.