Skip to content

Conversation

movitto
Copy link
Contributor

@movitto movitto commented Aug 1, 2013

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0%) when pulling d45cfa0 on movitto:create_partition into bf07ddd 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.

Suggest case statement instead of a complicated if

case fields[i]
when :start_sector, :end_sector, :size
  if val =~ /([0-9\.]*)([KMG])B/
    ...
  end
when :id
  val = val.to_i
end

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f8fee09 on movitto:create_partition into bf07ddd on ManageIQ:master.

@Fryguy
Copy link
Member

Fryguy commented Aug 5, 2013

Looks good to me. @jrafanie?

Copy link
Member

Choose a reason for hiding this comment

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

Minor nitpick: here we reference @Partitions and on line 74, 75, and 76 we reference partitions. I believe they're all referring to the same thing but here we access the instance variable directly whereas, the latter lines use the memoized partitions method. We should be consistent.

@jrafanie
Copy link
Member

jrafanie commented Aug 5, 2013

@movitto This looks awesome other than my minor nitpick above.

@coveralls
Copy link

Coverage Status

Coverage increased (+0%) when pulling e294073 on movitto:create_partition into bf07ddd on ManageIQ:master.

@jrafanie
Copy link
Member

jrafanie commented Aug 6, 2013

@movitto Very nice. When pushing commit(s) in response to feedback from someone, please @ mention them to let them know your commits are up and ready for re-review/merge. Thanks!

jrafanie added a commit that referenced this pull request Aug 6, 2013
@jrafanie jrafanie merged commit 6e93d41 into ManageIQ:master Aug 6, 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