Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix isA*Of exception messages #231

Merged
merged 1 commit into from
May 28, 2021

Conversation

smoench
Copy link
Contributor

@smoench smoench commented Jan 8, 2021

No description provided.

@Nyholm
Copy link
Collaborator

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

src/Assert.php Outdated Show resolved Hide resolved
@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
Collaborator

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.

tests/AssertTest.php Show resolved Hide resolved
tests/AssertTest.php Outdated Show resolved Hide resolved
@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 requested a review from Nyholm March 12, 2021 08:50
Copy link
Collaborator

@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