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

Switch to auto formatted code #314

Closed
bilogic opened this issue Apr 12, 2022 · 9 comments
Closed

Switch to auto formatted code #314

bilogic opened this issue Apr 12, 2022 · 9 comments

Comments

@bilogic
Copy link

bilogic commented Apr 12, 2022

Hi,

Can I suggest we use VSCode to auto format the codes? There are many benefits to this, namely:

  1. Easier for anyone to file clean PRs
  2. Higher readability, less work

I'm happy to file a PR.

@nelson6e65
Copy link
Contributor

VSCode? Did you mean ESLint / Prettier / PHPStan / PHPCS?

@bilogic
Copy link
Author

bilogic commented Apr 15, 2022

@nelson6e65 My bad, I looked. I'm using esbenp.prettier-vscode. I guess we should just pick one and make it known so that it is easier to make small changes in PRs.

@ondras what do you think?

@ondras
Copy link
Owner

ondras commented Apr 28, 2022

I am not sure this is going to solve a problem I am having. To address your arguments:

  1. Easier for anyone to file clean PRs

In what sense? I do not recall dismissing a PR due to it being "unclean".

2. Higher readability, less work

There is no work being mandated with respect to formatting. Obviously becoming consistent with the "code style status quo" is useful.

I am okay with a PR that somewhat normalizes whitespace (as long as it does not introduce style changes incompatible with my own coding style). But I do not plan on applying that manually and/or automatically to my further code base adjustments.

@bilogic
Copy link
Author

bilogic commented Apr 28, 2022

@ondras

Ok, when I used the word clean, what I meant was small or much smaller
See here #254, this was indicated as too big, I had a quick look and also felt it was beyond what I can afford, largely due to it being hard to read.

Reading to understand by itself is work :)

And the latest commit was only removing 5 lines, but there was another extra 16 additions and deletions (probably due to some auto formating of tabs to spaces etc)
4c2b6f5

It's just a suggestion, but would require core collaborators to stick to using the auto formatter, otherwise it would not bring any benefits. Thank you.

@ondras
Copy link
Owner

ondras commented Apr 28, 2022

See here #254, this was indicated as too big, I had a quick look and also felt it was beyond what I can afford, largely due to it being hard to read.

Right, but the related functionality has been implemented and merged separately. Also, in #254 (comment), I mentioned that I would welcome a separate white-space normalizing commit. This still holds.

And the latest commit was only removing 5 lines, but there was another extra 16 additions and deletions (probably due to some auto formating of tabs to spaces etc)

Yep. Is this is problem?

@bilogic
Copy link
Author

bilogic commented Apr 28, 2022

Ok, let me file a PR using the auto formatter esbenp.prettier-vscode, we see if it is suitable. Basically, whitespace additions and deletions require additional effort to parse (read and understand) and ignore, yet it is something that can be eliminated.

Thanks!

This was referenced Sep 11, 2022
@bilogic
Copy link
Author

bilogic commented Sep 11, 2022

Ok, I have done it, removes all white-spaces + auto format, the tools used are also listed.
All I basically did was open, save, close and commit all changes.

@ondras
Copy link
Owner

ondras commented Sep 15, 2022

Sorry for not replying earlier. I just accepted the PR. Thanks!

@bilogic
Copy link
Author

bilogic commented Sep 15, 2022

No worries, thanks! Now I can based my other PRs off this :)

@bilogic bilogic closed this as completed Sep 15, 2022
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

No branches or pull requests

3 participants