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

statement_indentation is too aggressive since 7384 PR #7497

Closed
mvorisek opened this issue Dec 3, 2023 · 6 comments · Fixed by #7624
Closed

statement_indentation is too aggressive since 7384 PR #7497

mvorisek opened this issue Dec 3, 2023 · 6 comments · Fixed by #7624
Assignees

Comments

@mvorisek
Copy link
Contributor

mvorisek commented Dec 3, 2023

Feature request

#7384 introduced support for comment for continuous control statement. As not everyone up for it, we can introduce rule configuration to opt in/out


Original content:

Bug report

introduced in #7384

the PR removed/updated several important tests like:

image

In short, it cannot be assumed:

if ($v) {
    // todo
}

comment only in if body is fine but

-perfectly fine code
+output after 7384 PR
 if ($v) {
     $u = 5;
-    // todo
+// todo
 } else {}

with comment at the end it is not. Comments below code can be still contextually valid and relevant to the previous code.

@mvorisek mvorisek changed the title statement_indentation is too agresive statement_indentation is too agresive since 7384 PR Dec 3, 2023
@Wirone
Copy link
Member

Wirone commented Dec 3, 2023

Please look at #7454, I agree with you but unfortunately @keradus not.

@mvorisek
Copy link
Contributor Author

mvorisek commented Dec 3, 2023

The fix is ok for a "really bad indentation", but as long as the comment is "indented in the body correctly", it must not be fixed. Given I am not alone who complaints, I belive it should be fixed.

It was reported also in #7454 (comment), #6947 and #6891, so even more people complain :)

@mvorisek mvorisek changed the title statement_indentation is too agresive since 7384 PR statement_indentation is too aggressive since 7384 PR Dec 3, 2023
@keradus
Copy link
Member

keradus commented Dec 3, 2023

Closing as duplicate of #7454 (comment)

If one wants to go left, and one wants to go right, of course - some ppl will complain regardless which solution you provide.
as PSR/PER not defining this, rule is following Sf standard.

I'm OK to have the rule configurable, feel free to bring that feature.

@mvorisek
Copy link
Contributor Author

mvorisek commented Dec 3, 2023

There are at least 4 unique people who want this to be fixed. Currently, the fix can change meaning, so we had to disable this rule...

Please keep this/one issue open. Either me or someone else can propose the fix.

@Wirone
Copy link
Member

Wirone commented Dec 3, 2023

As a side note: I really believe that changes like #7384 should not be merged without review.

@keradus
Copy link
Member

keradus commented Dec 3, 2023

@mvorisek I'm OK to have it open as feature request to have configurable option for the rule.
I'm not OK to change the behaviour fully opposite direction (again, ppl complain regardless which direction one goes with)

@Wirone , sometimes we have not enough code reviews, also not enough contributors. I dare to let myself permission to merge even if there is no review.

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