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.

Sign up for GitHub

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

Jump to bottom

Use SemicolonSpacing Sniff #53

Merged
merged 1 commit into from
Apr 9, 2018
Merged

Use SemicolonSpacing Sniff #53

merged 1 commit into from
Apr 9, 2018

Conversation

carusogabriel
Copy link
Contributor

@carusogabriel carusogabriel added this to the 5.0.0 milestone Apr 6, 2018
alcaeus
alcaeus previously approved these changes Apr 6, 2018
View reviewed changes
Majkl578
Majkl578 reviewed Apr 6, 2018
View reviewed changes
tests/input/not_spacing.php Outdated
@@ -5,11 +5,11 @@
$test = 1;

if (!$test > 0) {
echo 1;
echo 1 ;
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is not appropriate place for such tests, it is supposed to test NOT spacing.

Copy link
Member

Choose a reason for hiding this comment

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

@Majkl578 can you elaborate? I don't understand this comment

Copy link
Member

Choose a reason for hiding this comment

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

@Ocramius I suppose that @Majkl578 is saying that we're reusing a file which was intended to test the spacing after the negation operator (!)

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, that needs changing.

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'll also rename the file than, so new contributors can identify that better as well.

malarzm
malarzm previously approved these changes Apr 6, 2018
View reviewed changes
Ocramius
Ocramius previously approved these changes Apr 6, 2018
View reviewed changes
lcobucci
lcobucci previously requested changes Apr 6, 2018
View reviewed changes
Copy link
Member

@lcobucci lcobucci left a comment •
edited

Choose a reason for hiding this comment

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

As pointed out by @Majkl578 let's have new files to test new sniffs, that isolates things better and avoids unexpected results.

@carusogabriel carusogabriel dismissed stale reviews from Ocramius, malarzm, and alcaeus via 530b2fa April 7, 2018 19:14
lcobucci
lcobucci previously approved these changes Apr 7, 2018
View reviewed changes
greg0ire
greg0ire reviewed Apr 7, 2018
View reviewed changes
tests/input/semicolon_spacing.php
echo -1 ;

echo 'foo'
;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add an example with something multiline, like a chain of method calls, or maybe a heredoc, because that's the only case where I would consider adding a newline before a semicolon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like if that is even supported? :P

$qb->select()
    ->from()
    ->where()
    ;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greg0ire @Majkl578 That should be reported or not, in your opinions?

Copy link
Member

Choose a reason for hiding this comment

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

It should be reported and fixed IMO. Just my opinion though.

Copy link
Member

Choose a reason for hiding this comment

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

@greg0ire you mean "->where();" fixing or trimming-whitespaces-fixing? Personally I'm leaving ; in new line for builders and such so adding line doesn't require looking for semicolon and also diff is +1/0 instead of +2/-1 (similar to comma in arrays).

Copy link
Member

Choose a reason for hiding this comment

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

All in favor of switching to the former, please upvote this. All against, please downvote.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greg0ire You have my upvote, we just need to figure out how to deal with the Sniff. Also, can you please look at the fails? I can’t figure out what is wrong 👎

Copy link
Member

@greg0ire greg0ire Apr 9, 2018 •
edited

Choose a reason for hiding this comment

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

There must be an extra or missing newline somewhere… 8a9 sounds like it would be around like 8

Copy link
Member

Choose a reason for hiding this comment

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

Can we please leave it out of this specific patch and use the normal approval (and a dedicated PR) for this?

Copy link
Member

@greg0ire greg0ire Apr 9, 2018 •
edited

Choose a reason for hiding this comment

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

Can you really leave the part with the qb out if you have the part with

echo 'foo'
;

?

How is it different exactly? If we go with @malarzm proposal then we have an issue don't we?

Majkl578
Majkl578 reviewed Apr 8, 2018
View reviewed changes
tests/fixed/semicolon_spacing.php Outdated

$qb->select()
->from()
->where();
Copy link
Contributor

Choose a reason for hiding this comment

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

EOL is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that what CS is complaining, but I also couldn't identified what is :(

@carusogabriel carusogabriel force-pushed the semicolon-spaces branch 3 times, most recently from 3c8e43e to fb81f14 Compare April 9, 2018 02:37
@carusogabriel
Copy link
Contributor Author

PR ready for voting 🎉

malarzm
malarzm approved these changes Apr 9, 2018
View reviewed changes
Copy link
Member

@malarzm malarzm left a comment

Choose a reason for hiding this comment

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

I'd like to revisit the multiline stuff tho

@carusogabriel carusogabriel merged commit 0bd0268 into master Apr 9, 2018
@carusogabriel carusogabriel deleted the semicolon-spaces branch April 9, 2018 13:05
@carusogabriel carusogabriel self-assigned this Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Majkl578 Majkl578 Majkl578 left review comments

@greg0ire greg0ire greg0ire left review comments

@Ocramius Ocramius Ocramius approved these changes

@lcobucci lcobucci lcobucci approved these changes

@alcaeus alcaeus alcaeus approved these changes

@malarzm malarzm malarzm approved these changes

@jmikola jmikola Awaiting requested review from jmikola

@deeky666 deeky666 Awaiting requested review from deeky666

Assignees

@carusogabriel carusogabriel

Labels
New sniff RFC
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@carusogabriel @Majkl578 @Ocramius @lcobucci @alcaeus @greg0ire @malarzm