Skip to content

Conversation

carusogabriel
Copy link
Contributor

@carusogabriel
Copy link
Contributor Author

@kukulich Can I ask your help here? Am I missing something to get this to work?

@kukulich
Copy link
Contributor

kukulich commented Feb 1, 2018

@carusogabriel Hmm, in yesterday perspective I thought that the code should not be reported because there's already "early exit" code in if. It looks different to me in today perspective so I implemented it :) Try new build please.

@kukulich
Copy link
Contributor

kukulich commented Feb 1, 2018

@carusogabriel Just for your info, there are three types of errors in this sniff: "Use early exit instead of else.", "Remove useless else to reduce code nesting." and "Use early exit to reduce code nesting.". It may be good to document and test all of them here.

@carusogabriel
Copy link
Contributor Author

@kukulich Thanks, I'll add more test so

@Majkl578 Majkl578 added this to the 3.0.0 milestone Feb 5, 2018
@kukulich
Copy link
Contributor

kukulich commented Feb 5, 2018

https://github.com/slevomat/coding-standard/releases/tag/4.4.0

@carusogabriel
Copy link
Contributor Author

@kukulich Thanks. Time to update some tests and untag WIP 👷‍♂️

@carusogabriel carusogabriel changed the title [WIP] Early return sniff Early return sniff Feb 6, 2018
@Ocramius Ocramius self-assigned this Feb 6, 2018
@carusogabriel
Copy link
Contributor Author

Solving conflicts

README.md Outdated
- :white_check_mark: Omit phpDoc for parameters/returns with native types, unless adding description
- :white_check_mark: Don't use `@author`, `@since` and similar annotations that duplicate Git information
- :white_check_mark: Assignment in condition is not allowed
- :white_check_mark: Use early return instead of `else`
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is not true. The sniff does a little more :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have a suggestion? 😄

<rule ref="SlevomatCodingStandard.ControlStructures.DisallowYodaComparison"/>
<!-- Forbid weak comparisons -->
<rule ref="SlevomatCodingStandard.ControlStructures.DisallowEqualOperators"/>
<!-- Require usage of early exit instead of using else -->
Copy link
Contributor

Choose a reason for hiding this comment

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

@carusogabriel The comment is not true. The sniff does a little more :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have a suggestion? 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Ocramius
Copy link
Member

Ocramius commented Feb 6, 2018

@carusogabriel https://travis-ci.org/doctrine/coding-standard/jobs/337937694 failed :-\

@Ocramius Ocramius assigned carusogabriel and unassigned Ocramius Feb 6, 2018
@kukulich
Copy link
Contributor

kukulich commented Feb 6, 2018

@carusogabriel There's bad indentation here: https://github.com/doctrine/coding-standard/pull/22/files#diff-50034f4d68d4e08b523b28d23715cd5fR34

And nothing should change in method quoox. I think it's ok as it is.

@carusogabriel
Copy link
Contributor Author

carusogabriel commented Feb 6, 2018

@Ocramius @kukulich Thanks, was my editor dealing with tabs 🤦‍♂️

@Ocramius
Copy link
Member

Ocramius commented Feb 6, 2018

@carusogabriel yeah, still some weirdness going on there :P

@Ocramius
Copy link
Member

Ocramius commented Feb 6, 2018

Rebase also needed, sorry @carusogabriel!

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Ocramius Ocramius merged commit 2f3a075 into doctrine:master Feb 6, 2018
@Ocramius Ocramius assigned Ocramius and unassigned carusogabriel Feb 6, 2018
@carusogabriel carusogabriel deleted the early-returns branch February 6, 2018 11:03
@Ocramius
Copy link
Member

Ocramius commented Feb 6, 2018

@carusogabriel thanks! Proceeding to release :)

@carusogabriel
Copy link
Contributor Author

@Ocramius Perfect. Should we update across Doctrine's repos as well?

@Ocramius
Copy link
Member

Ocramius commented Feb 6, 2018

@carusogabriel yeah, let's do that afterwards 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants