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

Use tabs instead of spaces #4454

Merged
merged 8 commits into from
Jun 30, 2022
Merged

Use tabs instead of spaces #4454

merged 8 commits into from
Jun 30, 2022

Conversation

distantnative
Copy link
Member

@distantnative distantnative commented Jun 30, 2022

This PR …

I would suggest to update the config settings in this PR, update our local editors... but only update the actual files as we go along (would be a huge PR otherwise).

Refactored

Ready?

  • Unit tests for fixed bug/feature
  • In-code documentation (wherever needed)
  • Tests and checks all pass

For review team

@distantnative distantnative added the type: refactoring ♻️ Is about bad code; cleans up code label Jun 30, 2022
@distantnative distantnative self-assigned this Jun 30, 2022
@distantnative distantnative added the needs: discussion 🗣 Requires further discussion to proceed label Jun 30, 2022
@distantnative distantnative marked this pull request as ready for review June 30, 2022 12:58
@distantnative distantnative added this to the 3.7.1 milestone Jun 30, 2022
@distantnative distantnative requested a review from a team June 30, 2022 12:58
@distantnative distantnative added needs: delay ⏳️ Requires more time, on hold and removed needs: delay ⏳️ Requires more time, on hold labels Jun 30, 2022
@lukasbestle
Copy link
Member

Probably a good move. Accessibility is one of the most important factors in a decision like this. Most other arguments pro and contra tabs vs spaces are stupid in comparison.

There are only two potential issues we would need to check first:

  • It would be a violation of PSR-12. So I believe PHP-CS-Fixer will correct the indentation back every time. We need to check if PHP-CS-Fixer can be configured to use tabs. We would still not conform to the standard, but I guess the standard needs to change then, not us. ;)
  • In the past I had issues with lines that were manually indented to match previous or following lines. We need to check if we have any of those in our code. Rough example of what I mean:
    $thisIsAVariable = 'With a value that spans over multiple lines.' .
                       'This would break with tabs because it would require mixed indentation.';

Also not sure if we should update the files over time. We would have to disable all linting with Prettier and PHP-CS-Fixer. Also it will litter all PRs with whitespace changes that distract when reviewing the PR.

@distantnative
Copy link
Member Author

Apparently there is ->setIndent("\t") for PHP-CS-Fixer

@distantnative distantnative removed the needs: discussion 🗣 Requires further discussion to proceed label Jun 30, 2022
Copy link
Member

@bastianallgeier bastianallgeier left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

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

I think the XML files (PHPMD, PHPUnit, Psalm) use twice the amount of tabs as would be necessary. E.g. the first level uses two tabs.

The PHPMD file is also an example for what I meant with the "extra indentation". The attributes for the root tag <ruleset> are indented to match the previous lines. I think these should just be one attribute per line without the artificial breaks in the attr values. And then all attributes just indented with one tab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: refactoring ♻️ Is about bad code; cleans up code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants