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

Source code formatting. #708

Merged
merged 1 commit into from Oct 5, 2014

Conversation

Projects
None yet
6 participants
@TankOs
Member

TankOs commented Sep 30, 2014

  • Converted newline characters to \n.
  • Converted tabs to spaces.
@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 30, 2014

Member

Since this PR already touches lots of files regarding code formatting, I'd like to add one tiny request making source code formatting more consistent and which might enable automatic correction (tools like clang-format) easier to use:

Remove spaces in front of :, e.g. case sf::Event::SomEvent :, since this is really uncommon IMO (and probably related to Laurent being from France).

Member

MarioLiebisch commented Sep 30, 2014

Since this PR already touches lots of files regarding code formatting, I'd like to add one tiny request making source code formatting more consistent and which might enable automatic correction (tools like clang-format) easier to use:

Remove spaces in front of :, e.g. case sf::Event::SomEvent :, since this is really uncommon IMO (and probably related to Laurent being from France).

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 30, 2014

Member

Agreed, this is from a long time ago, I'm more strict with english punctuation now.

Member

LaurentGomila commented Sep 30, 2014

Agreed, this is from a long time ago, I'm more strict with english punctuation now.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Sep 30, 2014

Member

Shouldn't be a big deal, I will adjust this PR.

Member

TankOs commented Sep 30, 2014

Shouldn't be a big deal, I will adjust this PR.

@TankOs TankOs self-assigned this Sep 30, 2014

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 30, 2014

Member

So you replace all case X : with case X:? I'm fine with that, I also think that's more common.
There are also several comments that use a space before the colon, but this is not so crucial.

Keep in mind that you can't simply replace " :" with ":" because there are other cases (operator?:, inheritance, constructor initializer list) where the space is appropriate. If I can help with the conversion, let me know! With a bunch of regular expressions, it should be doable.

Member

Bromeon commented Sep 30, 2014

So you replace all case X : with case X:? I'm fine with that, I also think that's more common.
There are also several comments that use a space before the colon, but this is not so crucial.

Keep in mind that you can't simply replace " :" with ":" because there are other cases (operator?:, inheritance, constructor initializer list) where the space is appropriate. If I can help with the conversion, let me know! With a bunch of regular expressions, it should be doable.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Sep 30, 2014

Member

Check the PR, I've adjusted it. The regex was actually quite complex, because there are a couple of special cases. But I think I hit the correct ones. ;)

Member

TankOs commented Sep 30, 2014

Check the PR, I've adjusted it. The regex was actually quite complex, because there are a couple of special cases. But I think I hit the correct ones. ;)

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 30, 2014

Member

Didn't you want to add a separate commit for the colon change?
The entire diff contains almost 15k lines (!), GitHub won't even show everything... and I'll never find the needle in the haystack ;)

Edit: The previous SHA-1 was 923cc54. I split the commits in the new branch feature/newline_colons. The difference commit containing the colon changes is def0149. Feel free to adopt it if you want.

Member

Bromeon commented Sep 30, 2014

Didn't you want to add a separate commit for the colon change?
The entire diff contains almost 15k lines (!), GitHub won't even show everything... and I'll never find the needle in the haystack ;)

Edit: The previous SHA-1 was 923cc54. I split the commits in the new branch feature/newline_colons. The difference commit containing the colon changes is def0149. Feel free to adopt it if you want.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 30, 2014

Member

Not sure what gain we'd get from that, but I guess it couldn't hurt either... 😄

Member

eXpl0it3r commented Sep 30, 2014

Not sure what gain we'd get from that, but I guess it couldn't hurt either... 😄

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 30, 2014

Member

@LaurentGomila are you okay with those changes?

public :                      public:
protected :                   protected:
private :                     private:
case :                        case:
// comment : something        // comment: something

@eXpl0it3r Of course it's not an essential change, but it simplifies contributions by SFML developers and users, since it's a much more common convention.

@TankOs You forgot inheritance inside comments 😛
I fixed it in the other branch.

I wonder how many branches this will screw up... maybe let's merge the ones for SFML 2.2 first, otherwise they'll never be finished 😉

And I noticed there are further issues. Edit: Also fixed in feature/newline_colons.

  • stb_image shouldn't be modified, as it's an external library
  • in the Android code, the colons are aligned wrongly in switch
Member

Bromeon commented Sep 30, 2014

@LaurentGomila are you okay with those changes?

public :                      public:
protected :                   protected:
private :                     private:
case :                        case:
// comment : something        // comment: something

@eXpl0it3r Of course it's not an essential change, but it simplifies contributions by SFML developers and users, since it's a much more common convention.

@TankOs You forgot inheritance inside comments 😛
I fixed it in the other branch.

I wonder how many branches this will screw up... maybe let's merge the ones for SFML 2.2 first, otherwise they'll never be finished 😉

And I noticed there are further issues. Edit: Also fixed in feature/newline_colons.

  • stb_image shouldn't be modified, as it's an external library
  • in the Android code, the colons are aligned wrongly in switch
@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 30, 2014

Member

@LaurentGomila are you okay with those changes?

Sure, that's the way it should be, and the way I do it now.

Member

LaurentGomila commented Sep 30, 2014

@LaurentGomila are you okay with those changes?

Sure, that's the way it should be, and the way I do it now.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Sep 30, 2014

Member

There's a very very very small inconsistency: we have class A : B but struct A: B.

e.g. def0149#diff-0e23690458fb0b6ec9d876364d592a74L36

Member

mantognini commented Sep 30, 2014

There's a very very very small inconsistency: we have class A : B but struct A: B.

e.g. def0149#diff-0e23690458fb0b6ec9d876364d592a74L36

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 30, 2014

Member

There's a very very very small inconsistency: we have class A : B but struct A: B.

Probably a regexp issue ;)

Member

LaurentGomila commented Sep 30, 2014

There's a very very very small inconsistency: we have class A : B but struct A: B.

Probably a regexp issue ;)

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Sep 30, 2014

Member

Yep indeed, thanks for pointing it out. It's already adjusted in my script. I will push a new patch soon, with separate commits, so that you can check it separately.

Member

TankOs commented Sep 30, 2014

Yep indeed, thanks for pointing it out. It's already adjusted in my script. I will push a new patch soon, with separate commits, so that you can check it separately.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Sep 30, 2014

Member

@Bromeon
Thanks for pointing out the issues. I adjusted my script though, regarding your own commits.

I've uploaded the commits, open for review. Please do not merge yet, as I'd like to squelch the commits beforehand.

Member

TankOs commented Sep 30, 2014

@Bromeon
Thanks for pointing out the issues. I adjusted my script though, regarding your own commits.

I've uploaded the commits, open for review. Please do not merge yet, as I'd like to squelch the commits beforehand.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 30, 2014

Member

I had a look at it, it looks mostly good. Only a few times where the regex led to a misaligned switch/case statement. Thanks for the work!

Would be nice if you kept the newline and colon commits separate, otherwise the latter are lost in the masses. But of course you can squash the various iterations of colon adaptations :)

The question is whether we merge feature/newline now and make a lot of branches incompatible, or we merge it later and need to adapt this branch afterwards...

Member

Bromeon commented Sep 30, 2014

I had a look at it, it looks mostly good. Only a few times where the regex led to a misaligned switch/case statement. Thanks for the work!

Would be nice if you kept the newline and colon commits separate, otherwise the latter are lost in the masses. But of course you can squash the various iterations of colon adaptations :)

The question is whether we merge feature/newline now and make a lot of branches incompatible, or we merge it later and need to adapt this branch afterwards...

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 30, 2014

Member

The question is whether we merge feature/newline now and make a lot of branches incompatible, or we merge it later and need to adapt this branch afterwards...

This discussion has already been held (multiple times?). The conclusion was that it doesn't matter.

Member

eXpl0it3r commented Sep 30, 2014

The question is whether we merge feature/newline now and make a lot of branches incompatible, or we merge it later and need to adapt this branch afterwards...

This discussion has already been held (multiple times?). The conclusion was that it doesn't matter.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 2, 2014

Member

Yeah AFAIR the conclusion was that the commits to be merged will simply be adjusted.

@Bromeon Could you point out the misaligned case statements? I'd like to fix those.

Member

TankOs commented Oct 2, 2014

Yeah AFAIR the conclusion was that the commits to be merged will simply be adjusted.

@Bromeon Could you point out the misaligned case statements? I'd like to fix those.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 2, 2014

Member

@Bromeon Nevermind the "point out" thing, saw it. ;-)

Member

TankOs commented Oct 2, 2014

@Bromeon Nevermind the "point out" thing, saw it. ;-)

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 2, 2014

Member

Updated the PR:

Branch has been force-updated.

Member

TankOs commented Oct 2, 2014

Updated the PR:

Branch has been force-updated.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 2, 2014

Member

Any more complaints?

Member

TankOs commented Oct 2, 2014

Any more complaints?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 5, 2014

Member

Needs to be rebased 😑
After that, I'll merge it asap.

Member

eXpl0it3r commented Oct 5, 2014

Needs to be rebased 😑
After that, I'll merge it asap.

@TankOs TankOs added s:accepted and removed s:undecided labels Oct 5, 2014

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 5, 2014

Member

Rebased and squashed, ready for merge. Branch can be deleted when merged, I hold a local copy, together with single commits, in case there are any issues.

Member

TankOs commented Oct 5, 2014

Rebased and squashed, ready for merge. Branch can be deleted when merged, I hold a local copy, together with single commits, in case there are any issues.

@eXpl0it3r eXpl0it3r added this to the 2.2 milestone Oct 5, 2014

Source code changes.
* Changed newlines to \n.
* Removed whitespace before colons.
* Fixed several alignments.

@eXpl0it3r eXpl0it3r merged commit f24ca9a into master Oct 5, 2014

@eXpl0it3r eXpl0it3r deleted the feature/newline branch Oct 5, 2014

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 5, 2014

Member

Tanks! 🐈

Member

TankOs commented Oct 5, 2014

Tanks! 🐈

@TankOs TankOs removed their assignment Apr 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment