Conversation
Pull Request Test Coverage Report for Build 620
💛 - Coveralls |
|
No worries @laszlof. Whenever you have time. |
| * | ||
| * @param bool $bootable | ||
| */ | ||
| public function setBootable(bool $bootable) |
There was a problem hiding this comment.
Seems like this should have a default value (true). The name of the method insinuates that it makes the volume bootable, rather than necessarily setting a flag.
| */ | ||
| public function setBootable(bool $bootable) | ||
| { | ||
| $bootable = boolval($bootable); |
There was a problem hiding this comment.
Casting here would be more performant. Just splitting hairs here though.
| * | ||
| * @param array $options | ||
| * | ||
| * $options['status'] = (string) The volume status. |
There was a problem hiding this comment.
I think the = here is supposed to line up with the next 2 lines.
| */ | ||
| public function resetStatus(array $options) | ||
| { | ||
| $options += ['id' => $this->id]; |
There was a problem hiding this comment.
Might prefer array_merge here. If they pass in an id this isnt going to overwrite it (Maybe you want that?).
| public $contentType; | ||
|
|
||
| /** @var int */ | ||
| /** @var \DateTimeImmutable */ |
There was a problem hiding this comment.
This typehint cant be right. This is a Content-Length header, it should probably be a string (according to API spec)
https://developer.openstack.org/api-ref/object-store/#get-object-content-and-metadata
|
Looks good pending travis |
|
Thank you @laszlof |
3 participants
Some minor updates I cherry-picked from a project I've been working on.
You have time to do a quick code review @laszlof?