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: Introduce NoEmptyBlockFixer #7061

Open
wants to merge 57 commits into
base: master
Choose a base branch
from

Conversation

Wirone
Copy link
Member

@Wirone Wirone commented Jun 15, 2023

Fixes #3514, created by @ntzm, rebased and synced with current requirements by me 😉.

1 test case is failing, I will investigate only after initial green light that we actually want this fixer (@keradus said it's OK if it's marked as risky, and it is).

@Wirone

This comment was marked as outdated.

@ntzm
Copy link
Contributor

ntzm commented Jun 15, 2023

Wow, I never thought anyone would use my branch for anything, nice one :D


--- Original
+++ New
-<?php while ($foo) {}
Copy link
Member

Choose a reason for hiding this comment

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

while($itrator->consume()) {}

I wonder if it makes sense to have config which structures could be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be added later as improvement, I would rather leave it as is now.

doc/list.rst Outdated Show resolved Hide resolved
$this->doTest($expected, $input);
}

public static function provideTestFixCases(): iterable
Copy link
Member

Choose a reason for hiding this comment

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

<?php

echo 1;

{}

echo 2;

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixer must be a candidate for input code. - currently fixer works for loops, switches etc. Is it required to handle this case?

Copy link
Member

@keradus keradus Jul 5, 2023

Choose a reason for hiding this comment

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

I would imagine {} to be handled by NoEmptyBlockFixer, if we want to touch only loop, then we would have NoEmptyBodyBlockOfLoopFixer, but I would recommend to not have that very specialized rule that covers only some limited cases

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, I just don't think that rule must cover every possible scenario since the very beginning. It's not my code, I just refurbished it from the ancient branch, I would like to provide it "as-is" (not counting required alignments). Then we can improve it even more. Example with {} is such an edge case that in my opinion doesn't have to be handled now.

@Wirone Wirone self-assigned this Jun 20, 2023
@Wirone
Copy link
Member Author

Wirone commented Jul 11, 2023

@ntzm do you have an idea why nested if test case fails? I did not have time to dig it yet, maybe you can help make pipeline green?

@ntzm
Copy link
Contributor

ntzm commented Jul 11, 2023

Not sure sorry, I don't really have any capacity nowadays to work on anything PHP open source related 😢

@github-actions

This comment was marked as outdated.

@github-actions github-actions bot added status/to verify issue needs to be confirmed or analysed to continue and removed status/stale labels Oct 20, 2023
@Wirone Wirone changed the title feature: Introduce NoEmptyBlockFixer feat: Introduce NoEmptyBlockFixer Oct 31, 2023
@SpacePossum
Copy link
Contributor

SpacePossum commented Oct 31, 2023

As it fixes if and else blocks, should this fix elseif blocks as well?
seems elseif is already covered

As it fixes finally, should this fix catch blocks as well?
seems catch is already covered, but is missing bunch of test cases;

  • multiple catch types (catch(Foo|Bar $e){})
  • without assigning (catch(A){})

The name is "empty block" as enduser I would expect this to be fixed as well:
(raised by others, but this is the most obvious empty block usage so, IMHO, should be fixed by this rule)

<?php {}
echo 1;
{}
?>

@Wirone
Copy link
Member Author

Wirone commented Oct 31, 2023

@SpacePossum please look here 😉. I've rebased it only without any additional changes, will try to solve CI issues soon but I don't want to work on this PR more than it's required. Any improvements can be added later.

@SpacePossum

This comment was marked as off-topic.

@Wirone

This comment was marked as off-topic.

@SpacePossum

This comment was marked as off-topic.

@Wirone

This comment was marked as off-topic.

@SpacePossum

This comment was marked as off-topic.

@Wirone
Copy link
Member Author

Wirone commented Oct 31, 2023

@SpacePossum which question of yours I did not respond to 🤷‍♂️? In terms of empty {} I just redirected you to other thread where I already said I don't want to support it. It's not my code, I just refreshed @ntzm's work and I try to make it pass CI in my spare time. Then I will make final code review myself, as I feel more like reviewer here. I don't get what do you want from me, really 🙂.

I said I just did a rebase without any code changes to keep PR up-to-date so it's not marked as stale. When I will actually work on it, I'll take your comment into consideration. What's the problem here?

@Wirone Wirone removed the status/to verify issue needs to be confirmed or analysed to continue label Oct 31, 2023
@SpacePossum
Copy link
Contributor

Not sure about a "problem," but this PR doesn't look finished to me, no matter who was or is the author, so I can ask no?
I tried something here
Wirone#3
take or leave, really strange talk, feel free to tag me "off topic" again :)

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.

Empty block fixer
6 participants