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 short ternary stan errors #645

Merged
merged 22 commits into from
May 19, 2020
Merged

Conversation

shmax
Copy link
Contributor

@shmax shmax commented Apr 18, 2020

Hi guys, I'm back. I've resumed work on my lazy load PR, but I thought I'd warm up on something "easy". I went through the code base and replaced all the short ternary operators with appropriate alternatives. Let me know if I messed anything up.

@mfn
Copy link
Contributor

mfn commented Apr 18, 2020

Tests are all green, but can we be sure each and every change is technically correct? The semantics are different.

@shmax
Copy link
Contributor Author

shmax commented Apr 18, 2020

Tests are all green, but can we be sure each and every change is technically correct? The semantics are different.

They sure are! If you look at the changes, you'll see that I didn't just blindly replace every ?: with a ??. I tiptoed around strings in particular, and used different checks in those cases. I also carefully examined the native typehinting and PHPDoc annotations wherever they were available, and paid special attention to public methods. Whenever I was still in doubt I fired up the debugger and watched the values as they moved around. There's no guarantee I didn't miss anything (I'm going to review the changes again, myself), but as you say the tests do pass, and suffice it to say I am well aware that "the semantics are different".

@shmax
Copy link
Contributor Author

shmax commented Apr 18, 2020

Meanwhile, I'm not sure what's going on with the the linting. PHP_CodeSniffer evidently released a new version a few days ago (last green CI in this repo was at 3.5.4, it's now at 3.5.5), and now the linting check lit up the code base with dozens of Squiz.Arrays.ArrayDeclaration.ValueNoNewline errors, many of which are false (see squizlabs/PHP_CodeSniffer#2937)

@shmax
Copy link
Contributor Author

shmax commented Apr 18, 2020

Locked the code sniffer version until they issue a fix for squizlabs/PHP_CodeSniffer#2937

src/Error/Error.php Outdated Show resolved Hide resolved
src/Error/Error.php Outdated Show resolved Hide resolved
src/Validator/Rules/ValidationRule.php Outdated Show resolved Hide resolved
tests/Executor/DirectivesTest.php Outdated Show resolved Hide resolved
tests/Executor/DirectivesTest.php Outdated Show resolved Hide resolved
tests/Executor/DirectivesTest.php Outdated Show resolved Hide resolved
tests/Server/Psr7/PsrStreamStub.php Outdated Show resolved Hide resolved
@simPod simPod mentioned this pull request Apr 20, 2020
simPod pushed a commit that referenced this pull request Apr 24, 2020
@simPod
Copy link
Collaborator

simPod commented Apr 24, 2020

PHP_CodeSniffer evidently released a new version a few days ago (last green CI in this repo was at 3.5.4, it's now at 3.5.5), and now the linting check lit up the code base with dozens of Squiz.Arrays.ArrayDeclaration.ValueNoNewline errors, many of which are false

I went ahead and pushed this a2780a0 to avoid pollution in PRs

@shmax
Copy link
Contributor Author

shmax commented Apr 25, 2020

I went ahead and pushed this a2780a0 to avoid pollution in PRs

Ah, very helpful, thanks. I'll update my PRs in the morning. 👍

@shmax
Copy link
Contributor Author

shmax commented May 16, 2020

@simPod Is there anything I still need to do on this PR?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.001%) to 86.13% when pulling 372eecd on shmax:fix-short-ternary into 1f0ec24 on webonyx:master.

@simPod
Copy link
Collaborator

simPod commented May 16, 2020

@shmax I'll try to contact @vladar and ask if I can go forward with such PRs myself

@shmax
Copy link
Contributor Author

shmax commented May 17, 2020

That would be lovely, thank you.

Copy link
Member

@vladar vladar left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

Also, sorry for the lack of activity, I've been super busy recently. Looking forward to work on graphql-php as soon as I finish my current project at work.

@vladar vladar merged commit 7a7add0 into webonyx:master May 19, 2020
@shmax
Copy link
Contributor Author

shmax commented May 19, 2020

No problem, been rather crushed, myself.

Thank you both for reviewing!

@shmax shmax deleted the fix-short-ternary branch October 17, 2022 19:18
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.

5 participants