Skip to content

Add support of "auto" value for start_offset transformation parameter - #123

Merged
const-cloudinary merged 2 commits into
masterfrom
feature/start-offset-auto
Jun 26, 2018
Merged

Add support of "auto" value for start_offset transformation parameter#123
const-cloudinary merged 2 commits into
masterfrom
feature/start-offset-auto

Conversation

@shedar

@shedar shedar commented Jun 4, 2018

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread tests/CloudinaryTest.php Outdated
$this->assertNull($method->invoke(null, ""));
$this->assertNull($method->invoke(null, null));
// shouldn't support "auto" value
$this->assertNull($method->invoke(null, "auto"), "auto");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

last "auto" in this function is a message for assertNull. If you want to add custom message, make it more verbose. If it is a mistake, just remove it.

Comment thread tests/CloudinaryTest.php Outdated
{
$method = new ReflectionMethod('Cloudinary', 'norm_auto_range_value');
$method->setAccessible(true);
$pairs = [

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can reuse this array in test_norm_range_value.
Now we have 2 pretty much identical tests written in completely different manner. Which is confusing.
The solution you provided is much better than test_norm_range_value, it would be great to refactor it as well.

@const-cloudinary
const-cloudinary merged commit 46546ac into master Jun 26, 2018
@const-cloudinary
const-cloudinary deleted the feature/start-offset-auto branch June 26, 2018 08:59
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