Hi!
The code in the PHP source repo has a lot of random dangling whitespace
at the end of lines, which looks sloppy and produces some weird effects
in diffs because some editors auto-clean it and some do not. I'd like to
clean it up (it's easy to do automatically) in master, but this produces
a big patch that may also create some conflicts with existing pull reqs
(easily resolved, but still).
So, I'd like to ask if there are any objections to such cleanup? If not,
then I'll do it, probably next weekend.
Stas Malyshev
smalyshev@gmail.com
On Mon, Jan 5, 2015 at 11:58 PM, Stanislav Malyshev smalyshev@gmail.com
wrote:
Hi!
The code in the PHP source repo has a lot of random dangling whitespace
at the end of lines, which looks sloppy and produces some weird effects
in diffs because some editors auto-clean it and some do not. I'd like to
clean it up (it's easy to do automatically) in master, but this produces
a big patch that may also create some conflicts with existing pull reqs
(easily resolved, but still).
So, I'd like to ask if there are any objections to such cleanup? If not,
then I'll do it, probably next weekend.
No objection to whitespace cleanup. iirc git even has options to ignore
whitespace changes on merge/rebase.
It would be nice to also add a pre-commit hook stripping trailing
whitespace, to avoid this happening again.
Nikita
On Mon, Jan 5, 2015 at 11:58 PM, Stanislav Malyshev smalyshev@gmail.com
wrote:The code in the PHP source repo has a lot of random dangling
whitespace at the end of lines, which looks sloppy and produces some
weird effects in diffs because some editors auto-clean it and some
do not. I'd like to clean it up (it's easy to do automatically) in
master, but this produces a big patch that may also create some
conflicts with existing pull reqs (easily resolved, but still). So,
I'd like to ask if there are any objections to such cleanup? If not,
then I'll do it, probably next weekend.
Don't do it for just master, it makes automatic merging possibly a pain.
It should go to all "alive" branches, with properly merged up commits.
No objection to whitespace cleanup. iirc git even has options to
ignore whitespace changes on merge/rebase.It would be nice to also add a pre-commit hook stripping trailing
whitespace, to avoid this happening again.
That would change a commit hash, or cause an extra commit. No thanks.
cheers,
Derick
On Mon, Jan 5, 2015 at 11:58 PM, Stanislav Malyshev <smalyshev@gmail.com
wrote:
The code in the PHP source repo has a lot of random dangling
whitespace at the end of lines, which looks sloppy and produces some
weird effects in diffs because some editors auto-clean it and some
do not. I'd like to clean it up (it's easy to do automatically) in
master, but this produces a big patch that may also create some
conflicts with existing pull reqs (easily resolved, but still). So,
I'd like to ask if there are any objections to such cleanup? If not,
then I'll do it, probably next weekend.Don't do it for just master, it makes automatic merging possibly a pain.
It should go to all "alive" branches, with properly merged up commits.No objection to whitespace cleanup. iirc git even has options to
ignore whitespace changes on merge/rebase.It would be nice to also add a pre-commit hook stripping trailing
whitespace, to avoid this happening again.That would change a commit hash, or cause an extra commit. No thanks.
This is not how pre-commit hooks work.
If we only want to check (as opposed to automatically fix), it would be
enough to recommend enabling the default pre-commit hook in our git
guidelines.
Nikita
On Mon, Jan 5, 2015 at 11:58 PM, Stanislav Malyshev <smalyshev@gmail.com
wrote:
The code in the PHP source repo has a lot of random dangling
whitespace at the end of lines, which looks sloppy and produces some
weird effects in diffs because some editors auto-clean it and some
do not. I'd like to clean it up (it's easy to do automatically) in
master, but this produces a big patch that may also create some
conflicts with existing pull reqs (easily resolved, but still). So,
I'd like to ask if there are any objections to such cleanup? If not,
then I'll do it, probably next weekend.Don't do it for just master, it makes automatic merging possibly a pain.
It should go to all "alive" branches, with properly merged up commits.No objection to whitespace cleanup. iirc git even has options to
ignore whitespace changes on merge/rebase.It would be nice to also add a pre-commit hook stripping trailing
whitespace, to avoid this happening again.That would change a commit hash, or cause an extra commit. No thanks.
This is not how pre-commit hooks work.
If we only want to check (as opposed to automatically fix), it would be
enough to recommend enabling the default pre-commit hook in our git
guidelines.
Even better than a server side hook, far less intrusive and rejecting
a commit when CS are broken before it even gets applied locally sounds
like the best solution so far.
For the reference, how to make it:
http://lostechies.com/jasonmeridth/2009/09/10/git-local-pre-commit-hook/
Now we only need a lint cmd call checking the PHP internals CS :)
--
Pierre
@pierrejoye | http://www.libgd.org
De : Pierre Joye [mailto:pierre.php@gmail.com]
Even better than a server side hook, far less intrusive and rejecting
a commit when CS are broken before it even gets applied locally sounds
like the best solution so far.
If it is technically possible, I think we need both.
Local-side pre-commit hook is optional and would just be a developer's help.
Server-side hook should be able to refuse pushes (with probably less strict rules).
François
So, I'd like to ask if there are any objections to such cleanup? If not,
then I'll do it, probably next weekend.
The bigger question is perhaps how they got added in the first place.
(And it's a bit like replacing all the tabs with spaces.) I've been
hitting patches that show a number of changes, but on closer inspection
it IS just a matter that a few odd lines have extra/different white
space. Having modified the merge process on my own repos so that
aesthetic changes are skipped keeps my own code tidier, but the master
copies on github still have the extras.
Basically - once the code has been swept can new patches be
automatically cleaned or will that still be a code review job?
--
Lester Caine - G8HFL
Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk
Hey Stas,
The code in the PHP source repo has a lot of random dangling whitespace
at the end of lines, which looks sloppy and produces some weird effects
in diffs because some editors auto-clean it and some do not. I'd like to
clean it up (it's easy to do automatically) in master, but this produces
a big patch that may also create some conflicts with existing pull reqs
(easily resolved, but still).
So, I'd like to ask if there are any objections to such cleanup? If not,
then I'll do it, probably next weekend.
I expect you’ll cause me considerable pain when I have to merge that into the bigints branch. But it’s for the best. I’m actually pretty guilty of whitespace changes myself, so I probably deserve that pain.
No objections. :)
--
Andrea Faulds
http://ajf.me/
Hi!
The code in the PHP source repo has a lot of random dangling whitespace
at the end of lines, which looks sloppy and produces some weird effects
in diffs because some editors auto-clean it and some do not. I'd like to
clean it up (it's easy to do automatically) in master, but this produces
a big patch that may also create some conflicts with existing pull reqs
(easily resolved, but still).
So, I'd like to ask if there are any objections to such cleanup? If not,
then I'll do it, probably next weekend.
Thanks for asking :)
And go ahead, they should not be there in the 1st place, we have CS
for a reason :)
That reminds me something I was wondering, would it be possible to
have a kind of CS check on push? :)
Cheers,
Pierre
@pierrejoye | http://www.libgd.org
Hey
That reminds me something I was wondering, would it be possible to
have a kind of CS check on push? :)
Yep - PHPCS1 is quite good, easy to configure and has good editor
integrations in the community. The automated code standards check can be
achieved using git-hooks2 which is already provided to us just by using
git. One thing to keep in mind is that the PHP lint is only run on files
you modified, not all within the repository.
Stas, let me know if you would like a hand implementing/cleaning up this
repository as I am happy wqto help.
Hi!
The code in the PHP source repo has a lot of random dangling whitespace
at the end of lines, which looks sloppy and produces some weird effects
in diffs because some editors auto-clean it and some do not. I'd like to
clean it up (it's easy to do automatically) in master, but this produces
a big patch that may also create some conflicts with existing pull reqs
(easily resolved, but still).
So, I'd like to ask if there are any objections to such cleanup? If not,
then I'll do it, probably next weekend.Thanks for asking :)
And go ahead, they should not be there in the 1st place, we have CS
for a reason :)That reminds me something I was wondering, would it be possible to
have a kind of CS check on push? :)Cheers,
Pierre
@pierrejoye | http://www.libgd.org
Hey
That reminds me something I was wondering, would it be possible to
have a kind of CS check on push? :)Yep - PHPCS1 is quite good, easy to configure and has good editor
integrations in the community. The automated code standards check can be
achieved using git-hooks2 which is already provided to us just by using
git. One thing to keep in mind is that the PHP lint is only run on files you
modified, not all within the repository.Stas, let me know if you would like a hand implementing/cleaning up this
repository as I am happy wqto help.
I did not PHPCS supports C. Or am I missing something?
I did not PHPCS supports C. Or am I missing something?
Nope, you are spot on :) - I read this as "let's clean up our
repositories" not just php-src (which reading back through was my
misinterpretation).
Do you have a tool in mind? I see there are a few like indent1,
uncrustify2 and astyle3 but I have not used any of them on a project
of significant size so I am unsure of the pros and cons of each.
Hey
That reminds me something I was wondering, would it be possible to
have a kind of CS check on push? :)Yep - PHPCS1 is quite good, easy to configure and has good editor
integrations in the community. The automated code standards check can be
achieved using git-hooks2 which is already provided to us just by using
git. One thing to keep in mind is that the PHP lint is only run on files you
modified, not all within the repository.Stas, let me know if you would like a hand implementing/cleaning up this
repository as I am happy wqto help.I did not PHPCS supports C. Or am I missing something?
That's what I had in mind:
https://gist.github.com/kblomqvist/bb59e781ce3e0006b644
rules to be adapted :)
--
Pierre
@pierrejoye | http://www.libgd.org