Skip to content

Conversation

FernandoOjeda
Copy link
Contributor

Cancel block storage issue #880

@coveralls
Copy link

coveralls commented Jun 19, 2018

Coverage Status

Coverage increased (+0.003%) to 88.142% when pulling 64fe8b0 on FernandoOjeda:fo_cancel_block_storage into 32a6e43 on softlayer:master.

@allmightyspiff allmightyspiff self-requested a review June 19, 2018 18:19
Copy link
Member

@allmightyspiff allmightyspiff left a comment

Choose a reason for hiding this comment

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

To get the tox -e analysis check to pass, heres how you handle the following complaints.

C:515, 0: Unnecessary parens after 'print' keyword (superfluous-parens)
Should be removed once you switch from print to throw exception

C:513, 8: Variable name "billingItem" doesn't conform to snake_case naming style (invalid-name)

change billingItem to billing_item

R:501, 4: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)
Removing the else: block should fix this.

Also, please add a unit test for this new if condition so test coverage doesn't go down.

Thanks :)

billing_item_id = block_volume['billingItem']['id']
billingItem = 'billingItem' in block_volume
if billingItem is False:
print('The block storage was cancelled')
Copy link
Member

Choose a reason for hiding this comment

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

The managers should not print out anything. I'd rather we raise an exception here.
https://github.com/softlayer/softlayer-python/blob/master/SoftLayer/managers/hardware.py#L87 has a good example of how to handle this sort of logic.

billingItem = 'billingItem' in block_volume
if billingItem is False:
print('The block storage was cancelled')
else:
Copy link
Member

Choose a reason for hiding this comment

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

Since were going to throw an exception if we don't have a billing item, the else bits are not really needed and can be reverted back.

@allmightyspiff allmightyspiff merged commit c05f4a0 into softlayer:master Jun 20, 2018
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