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: allow arbitrary space indentation #7818

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Rasmus-Bertell
Copy link

@Rasmus-Bertell Rasmus-Bertell commented Feb 4, 2024

I created this PR to address the issue in #7810.

Removes the limitation of having only 2 or 4 space indentation and allows for any number of spaces or one tab in the indentation settings.

This could be modified to also allow for any number of tabs but I don't see a reason for it since tabs are already arbitrary sized.

@Rasmus-Bertell Rasmus-Bertell changed the title Feature: Allow arbitrary space indentation feat: allow arbitrary space indentation Feb 4, 2024
@coveralls
Copy link

coveralls commented Feb 4, 2024

Coverage Status

coverage: 94.753%. remained the same
when pulling 9bd8035 on Rasmus-Bertell:feature/allow-arbitrary-space-indentation
into fe8f7fd on PHP-CS-Fixer:master.

@Wirone Wirone requested a review from keradus February 4, 2024 16:16
Copy link
Member

@keradus keradus left a comment

Choose a reason for hiding this comment

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

let's solve the discussion before concluding on PR

#7810

@@ -25,8 +25,8 @@ final class WhitespacesFixerConfig

public function __construct(string $indent = ' ', string $lineEnding = "\n")
{
if (!\in_array($indent, [' ', ' ', "\t"], true)) {
throw new \InvalidArgumentException('Invalid "indent" param, expected tab or two or four spaces.');
if (!Preg::match('/^(?: +|\t)$/', $indent)) {
Copy link
Member

@keradus keradus Feb 4, 2024

Choose a reason for hiding this comment

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

IF we would go forward direction to approve the PR, we need a new tests to prove it works:

  • each fixer that uses WhitespacesFixerConfig should receive a new test with new whitespace variant(s)
  • dedicated integration tests for new whitespace variant(s) (at least mimic tests/Fixtures/Integration/set/@PSR12_whitespaces.test*.php)

(i suggest to not invest into it till aligned into direction)

Copy link
Author

Choose a reason for hiding this comment

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

I'll convert this to a draft while I familiarize myself more with the codebase.

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

Successfully merging this pull request may close these issues.

None yet

3 participants