Skip to content

Conversation

@smoench
Copy link
Contributor

@smoench smoench commented Jan 8, 2021

No description provided.

@Nyholm
Copy link
Contributor

Nyholm commented Jan 18, 2021

Could you please rebase this PR?

@smoench smoench force-pushed the fix-isa-exception-message branch 2 times, most recently from 7450583 to 37db12a Compare January 18, 2021 12:34
@smoench
Copy link
Contributor Author

smoench commented Jan 18, 2021

PR is rebased

@smoench smoench force-pushed the fix-isa-exception-message branch 2 times, most recently from 9ca859e to e7c5462 Compare January 19, 2021 07:47
@smoench
Copy link
Contributor Author

smoench commented Jan 29, 2021

I also changed some error messages. You may have a look again?

$message ?: 'Expected an instance of this class or to this class among his parents %2$s. Got: %s',
static::typeToString($value),
$message ?: 'Expected an instance of this class or to this class among its parents "%2$s". Got: %s',
static::valueToString($value),
Copy link
Contributor

Choose a reason for hiding this comment

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

I still feel that typeToString() is correct here.

I want to see string, AcmeService or boolean. I don't want to see the string value. Do I?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I think it depends whether the value is passed as an object or string. WDYT about the following?

$isValueAString = \is_string($value);

if (!\is_a($value, $class, $isValueAString)) {
    static::reportInvalidArgument(sprintf(
        $message ?: 'Expected an instance of this class or to this class among its parents "%2$s". Got: %s',
        $isValueAString ? static::valueToString($value) : static::typeToString($value),  
        $class
    ));
}

An assertion about checking the type of value whether it is an object or string might be also a good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the invalid test cases with passing an object and static::valueToString($value) is correct here.

@smoench smoench force-pushed the fix-isa-exception-message branch 2 times, most recently from 355d491 to 00c780f Compare March 10, 2021 07:37
@smoench smoench force-pushed the fix-isa-exception-message branch from 00c780f to aede6e6 Compare March 10, 2021 07:42
@smoench smoench requested a review from Nyholm March 12, 2021 08:50
Copy link
Contributor

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this.

@Nyholm Nyholm merged commit 8403a94 into webmozarts:master May 28, 2021
@smoench smoench deleted the fix-isa-exception-message branch May 28, 2021 09:36
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