Skip to content

Conversation

@paulbalandan
Copy link
Member

@paulbalandan paulbalandan commented Jun 3, 2021

Description
Depends on #4770 .

This enables the indentation_type rule to convert usages of tabs to 4 spaces for indentations.

Note: Changes are purely conversion of tabs to spaces as generated by php-cs-fixer. No other manual changes done except for the errant \App\Config\App used in Config\Reader in admin/module/.

Checklist:

  • Securely signed commits
  • Conforms to style guide

@sfadschm
Copy link
Contributor

sfadschm commented Jun 3, 2021

So CI4 will now be using spaces for both indentation and alignment?

@paulbalandan paulbalandan force-pushed the convert-tabs-to-spaces branch from 0eb7bb1 to c91ee66 Compare June 3, 2021 15:51
@paulbalandan
Copy link
Member Author

So CI4 will now be using spaces for both indentation and alignment?

Per discussion, we'll start adopting PSR-12.

@paulbalandan paulbalandan requested a review from lonnieezell June 3, 2021 15:57
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

Looks good from what I can tell. My knowledge of Nexus is slim, but everything else seems appropriate.

@samsonasik
Copy link
Member

composer.json seems conflict with latest change by dependabot.

@paulbalandan paulbalandan force-pushed the convert-tabs-to-spaces branch from c91ee66 to aa34bd2 Compare June 4, 2021 14:18
@paulbalandan paulbalandan force-pushed the convert-tabs-to-spaces branch from aa34bd2 to e60f74b Compare June 4, 2021 14:29
@paulbalandan paulbalandan merged commit e04d17b into codeigniter4:develop Jun 4, 2021
@paulbalandan paulbalandan deleted the convert-tabs-to-spaces branch June 4, 2021 14:51
@totoprayogo1916
Copy link
Contributor

@totoprayogo1916
Copy link
Contributor

in CI4.1.3, framework & sppstarter still use tabs.

@paulbalandan
Copy link
Member Author

in CI4.1.3, framework & sppstarter still use tabs.

This is intended. These changes will have a dedicated release.

@lighth7015
Copy link

Honestly this doesn't seem like a good idea. For example accessibility will suffer (tabs allow users to set their preferred indentation according to the settings that make sense for them. Forcing spaces may have a detrimental effect for those people.

@paulbalandan
Copy link
Member Author

Hi! Thanks for your insight. However, this conversion to spaces applies only to the framework code. End users are not forced to convert their existing code to use spaces. They can still use their preference here. As such, I see no detrimental effect of this to end users as they are not directly affected. Again, this change is internal.

@gnat
Copy link

gnat commented Apr 15, 2022

Long looong time Code Igniter user here going back to 2008 with multiple major sites on Code Igniter. Got the EllisLab tattoo and all...

I would highly advise a revert as it forces a style change of everything built on Code Igniter going back to V1.

Hi! Thanks for your insight. However, this conversion to spaces applies only to the framework code. End users are not forced to convert their existing code to use spaces. They can still use their preference here. As such, I see no detrimental effect of this to end users as they are not directly affected. Again, this change is internal.

Why even do it then? Seriously restrain yourself from making changes to just "pad out" your github man.

Again, this change is internal.

Nobody here modifies Code Igniter or writes add-ons? Your "contribution" just wonked up the entire Code Igniter ecosystem. What was the win?

@gnat
Copy link

gnat commented Apr 15, 2022

On that note I am disliking this recent trend of people joining a project just to add/remove white space. It ends up being quite disruptive to those who use the projects.

@kenjis
Copy link
Member

kenjis commented Apr 15, 2022

@kenjis
Copy link
Member

kenjis commented Apr 15, 2022

Why even do it then?

It is because all maintainers agreed with switching to PSR-12.

@lonnieezell
Copy link
Member

I have also used CodeIgniter since pretty close to that same time, version 1.6 IIRC. Glad to see you still around!

I would highly advise a revert as it forces a style change of everything built on Code Igniter going back to V1.

This doesn't force a style change on anything except the internal code for CodeIgniter. Developers not contributing code to the framework itself are not affected at all. I understand tabs vs spaces is a hot topic for a lot of people. I used to be on the tabs side, myself. Over the years I've found that using a decent IDE makes it not matter anymore.

Why even do it then? Seriously restrain yourself from making changes to just "pad out" your github man.

We chose to adopt PSR-12 to take advantage of the many bits of tooling and automation that can come with that. We understand the PSR's are not for everyone, and they were never meant to be. They were specifically developed to aid in framework interoperability.

Nobody here modifies Code Igniter or writes add-ons? Your "contribution" just wonked up the entire Code Igniter ecosystem. What was the win?

Again - unless you are contributing code to the core of the framework, this really doesn't affect you. If you write addons you can format it however you choose. If you edit the core files, then there might be a small irritation and that's understandable. But there's usually better solutions than modifying the core for your project.

You don't have to like spaces, which obviously you don't, and that's totally cool. Continuing using tabs in your own code and enjoy.

@gnat
Copy link

gnat commented Apr 15, 2022

Thank you for the chill responses. It's reassuring to hear this was by consensus of the current team of maintainers.

Yeah, Code Igniter is still one of my all time favorites. Glad to see people still passionate about it. Was pretty shocked to do a pull and see this major changeset on every file in the repo. It is what it is.

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.

8 participants