Skip to content

Combine consecutives unsets#2942

Merged
Ocramius merged 1 commit intodoctrine:masterfrom
carusogabriel:combine-consecutives-unsets
Dec 20, 2017
Merged

Combine consecutives unsets#2942
Ocramius merged 1 commit intodoctrine:masterfrom
carusogabriel:combine-consecutives-unsets

Conversation

@carusogabriel
Copy link
Copy Markdown
Contributor

unset allow us to pass multiple values to check.

PHP-CS-Fixer helped me on that.

@Majkl578
Copy link
Copy Markdown
Contributor

I have mixed feelings about this one, it makes it worse to read... :/

@carusogabriel
Copy link
Copy Markdown
Contributor Author

I did it to reduce lines, but if readability is more important, why not leave it as it is? 😄

@Majkl578
Copy link
Copy Markdown
Contributor

I personally think it is, but let @Ocramius decide. :)

@Ocramius
Copy link
Copy Markdown
Member

@Majkl578 the readability is quite OK, I just think we aren't used to variadic isset/unset. I myself learned about it just 2 years ago, and I do PHP since aaaaaaages (sadly).

The change is semantically more correct, as this is an atomic operation, so a grouped unset is better than a sequence of unset calls anyway.

@Ocramius Ocramius self-assigned this Dec 20, 2017
@Ocramius Ocramius added this to the 2.7.0 milestone Dec 20, 2017
@Ocramius Ocramius merged commit 9f843bc into doctrine:master Dec 20, 2017
@carusogabriel carusogabriel deleted the combine-consecutives-unsets branch December 20, 2017 11:34
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants