Skip to content

Conversation

movitto
Copy link
Contributor

@movitto movitto commented Jul 17, 2013

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0%) when pulling 118cb1f on movitto:partition_format_to into 42d0486 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 Since I was confused at first, does it make sense to not use a local variable with the same name as the instance variable and attr_accessor getter method?

Maybe pass in filesystem or rename the attr_accessor to filesystem and keep the parameter as fs_type.

@movitto
Copy link
Contributor Author

movitto commented Jul 17, 2013

it make sense to not use a local variable with the same name as the instance variable and attr_accessor getter method

In general I'm not against doing this, afterall thats what the heiroglyphs (eg '@' , '$', etc) are for and using the same variable name indicates that that value refers to the same bit of data (if you wanted to call the accessor in that method you could do self.fs_type = fs_type). In any case, nbd so updated.

Do we need to capture any output/errors and handle/return them here?

Not sure, but until we have a specific need, rather leave this as is. Right now if the operation fails this will error out (as intended), and there are alot of edge cases which we could address (from running out of memory, to specific command exceptions) to the point it gets cluttered. If better error handling is required in the future, we can always add it then

@jrafanie
Copy link
Member

Good point... I'll be testing this stuff anyway as I start using it so we can add error handling once we know more information.

jrafanie added a commit that referenced this pull request Jul 17, 2013
@jrafanie jrafanie merged commit 28d869d into ManageIQ:master Jul 17, 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.

3 participants