Skip to content

Conversation

movitto
Copy link
Contributor

@movitto movitto commented Aug 21, 2013

No description provided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really belong in LinuxAdmin::Common? I'm thinking it might be better to have a Volume class that acts as a base class for LogicalVolume and PhysicalVolume.

@Fryguy
Copy link
Member

Fryguy commented Aug 21, 2013

Why were all of the described_class calls undone?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.18%) when pulling 849b27d on movitto:code-cleanup1 into 9f8d568 on ManageIQ:master.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@movitto Does it make sense to process each line in a method? You might have to splat the *process_volume_display_line.

def self.process_volume_display_line(line)
  fields = line.split(':')
  vgname = fields[1] 
  vg = vgs.find { |vg| vg.name == vgname }
  return fields, vg 
end

def self.scan_volumes(cmd)
  # ...
  out.each_line do |line|
    fields, vg = process_volume_display_line(line)
    lvs << yield(fields, vg)
  end

  lvs
end

@jrafanie
Copy link
Member

@movitto This looks really good so far. As long as we bury the complicated logic like what 8-10 columns in a fstab line mean in a nicely named method, the callers don't have to care the order of those columns or if they are chomped, split, and String/Fixnums, etc.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 76d6ec7 on movitto:code-cleanup1 into 9f8d568 on ManageIQ:master.

@movitto
Copy link
Contributor Author

movitto commented Aug 22, 2013

@jrafanie @Fryguy

  • added volume subclass w/ suggested helper methods
  • pulled out fstab entry processing to a FStabEntry.from_line helper method
  • renamed get to read
  • probably forgetting something :-)

@Fryguy
Copy link
Member

Fryguy commented Aug 22, 2013

This PR is not mergable, Please rebase and repush.

@movitto
Copy link
Contributor Author

movitto commented Aug 22, 2013

@jrafanie @Fryguy rebased

@coveralls
Copy link

Coverage Status

Coverage increased (+0.18%) when pulling 3bf7ec0 on movitto:code-cleanup1 into 350b850 on ManageIQ:master.

jrafanie added a commit that referenced this pull request Aug 22, 2013
Code cleanup for code climate
@jrafanie jrafanie merged commit d275f50 into ManageIQ:master Aug 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants