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

feat: require non-capturing catch #281

Merged
merged 1 commit into from Jul 18, 2022
Merged

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Jul 16, 2022

to avoid unused redundant variables in catch blocks

@simPod simPod marked this pull request as ready for review July 16, 2022 12:40
@simPod simPod requested a review from a team as a code owner July 16, 2022 12:40
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, although very confusing inputs (don't see any diffs in the tests either 🤔 )

Comment on lines +1 to +5
<?php

declare(strict_types=1);

namespace Exceptions;
Copy link
Member

Choose a reason for hiding this comment

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

Are these files supposed to have no body? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, it's a base file and the body is added via 80 and 81 patches. No support in < 8

Copy link
Member

Choose a reason for hiding this comment

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

let's just drop php 7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree but not my call. Let's have @doctrine team do that.

Copy link
Member

@greg0ire greg0ire Jul 16, 2022

Choose a reason for hiding this comment

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

Honestly, why not? We run the cs jobs on PHP 8, and can use ^9.0 || ^10.0 as a version constraint on projects that need to support PHP 7.

Copy link
Member

Choose a reason for hiding this comment

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

Ah but I get why you would be reluctant to do so: #226

@beberlei , if the code still works, but the tests are harder to write, can we maybe drop support for PHP 7?

Copy link
Member

Choose a reason for hiding this comment

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

This repository does not contain any code, so I don't see a point in requiring a higher PHP version than the sum of our dependencies. @greg0ire is right that we don't run PHPCS itself on PHP 7 anymore, but we run it on code that needs to run on PHP 7.

What the test suite currently covers isn't running the ruleset on different PHP versions, but running it for different language levels. And we would still need that matrix, even if we bumped the min PHP version in composer.json to 8.2.

I understand that reviewing those patch files is cumbersome. Maybe it would be a little less painful if we created dedicated fixed folders per language level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it would be a little less painful if we created dedicated fixed folders per language level?

Inputs are also different for different PHP versions.

Copy link
Member

@derrabus derrabus Jul 18, 2022

Choose a reason for hiding this comment

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

I see. And do you think it would help to reverse the patching process? We'd keep all fixtures in the highest supported PHP language level (8.1 currently) and create patches for the lower levels? My expectation would be that PRs would become easier to review and it would be easier to drop old PHP versions eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maintenance would still be PIA (since maintaining eoled phps sucks and there's too many versions already) but it may facilitate reviews, yes.

@greg0ire greg0ire merged commit e64b67f into doctrine:10.0.x Jul 18, 2022
@greg0ire
Copy link
Member

Thanks @simPod !

@simPod simPod deleted the non-capture branch July 19, 2022 07:49
simPod added a commit to cdn77/coding-standard that referenced this pull request Sep 20, 2022
simPod added a commit to cdn77/coding-standard that referenced this pull request Sep 20, 2022
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.

None yet

6 participants