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

Actually check that __toString method exists #56

Merged
merged 1 commit into from
Aug 11, 2018

Conversation

teohhanhui
Copy link
Contributor

@teohhanhui teohhanhui commented Mar 28, 2018

Check before we try to cast to string.

@teohhanhui teohhanhui changed the title Actually check that _toString method exists Actually check that __toString method exists Mar 28, 2018
Copy link
Owner

@ramsey ramsey left a comment

Choose a reason for hiding this comment

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

isValid() can also verify whether a string $value is a valid UUID, so I think the || and && conditions need to be reworked here.

What might be best is to check whether it’s an instance of a UuidInterface, and in the UuidInterface define the __toString() method as part of the required class interface.

@teohhanhui
Copy link
Contributor Author

Fixed.

@ramsey
Copy link
Owner

ramsey commented Apr 20, 2018

The build is failing because the line is too long. Do you mind breaking that statement into multiple lines?

Check before we try to cast to string
@teohhanhui
Copy link
Contributor Author

I've split it into multiple lines, but do you prefer trailing conditionals or the other way round? :)

@ramsey
Copy link
Owner

ramsey commented Apr 30, 2018

The way you've formatted it is fine.

Oops! One more error:

 84 | ERROR | [x] Expected 0 spaces after opening bracket; newline
    |       |     found
    |       |     (PSR2.ControlStructures.ControlStructureSpacing.SpacingAfterOpenBrace)

You can run composer test locally before pushing to see what all the build will complain on, if there are errors.

@teohhanhui
Copy link
Contributor Author

I think we need to update the phpcs rule. I'm following the draft PSR-12: https://github.com/php-fig/fig-standards/blob/master/proposed/extended-coding-style-guide.md#51-if-elseif-else

Expressions in parentheses MAY be split across multiple lines, where each subsequent line is indented at least once. When doing so, the first condition MUST be on the next line. The closing parenthesis and opening brace MUST be placed together on their own line with one space between them. Boolean operators between conditions MUST always be at the beginning or at the end of the line, not a mix of both.

<?php

if (
    $expr1
    && $expr2
) {
    // if body
} elseif (
    $expr3
    && $expr4
) {
    // elseif body
}

@teohhanhui
Copy link
Contributor Author

teohhanhui commented May 4, 2018

Anyway, that phpcs rule is wrong. PSR-2 does not care if you add a newline. It only cares about space: https://www.php-fig.org/psr/psr-2/#5-control-structures

There MUST NOT be a space after the opening parenthesis

@ramsey ramsey merged commit 0e00030 into ramsey:master Aug 11, 2018
@ramsey
Copy link
Owner

ramsey commented Aug 11, 2018

There MUST NOT be a space after the opening parenthesis

A newline character is technically a space character. 😄

At any rate, I have updated the project to use PHP CodeSniffer 3.3.1 and the new PSR-12 sniffs. Unfortunately, phpcs currently has a bug with the specific rule that covers control structure spacing, so I'm ignoring that rule for now:

Thanks for your contribution! 🎉

@ramsey
Copy link
Owner

ramsey commented Aug 11, 2018

Oops! Now, it should be passing on Travis CI. f139239

😄

@teohhanhui
Copy link
Contributor Author

A newline character is technically a space character.

Space is space. Whitespace is whitespace. Space is whitespace yes, but whitespace is not space. 😂

@teohhanhui teohhanhui deleted the patch-1 branch August 11, 2018 20:03
@ramsey
Copy link
Owner

ramsey commented Aug 11, 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.

2 participants