search

erikdoe / ocmock

Mock objects for Objective-C
http://ocmock.org
Apache License 2.0
2.16k stars 606 forks source link

Remove extra whitespaces for whitespaces only lines #428

Closed matrush closed 3 years ago

matrush commented 4 years ago

These extra whitespaces are more than annoying if your editor happened to be configured to trim these automatically on save. A lot of modern editors does this automatically such as VSCode and Xcode.

To save the pain from revert unwanted changes every time, I decided to fix this once and for all :)

matrush commented 4 years ago

@erikdoe Thoughts on this one? I feel it's better to remove these personally...

erikdoe commented 3 years ago

Apologies, this has been open for quite a while without a comment. In general I like the idea and I thought about cleaning up spaces for a while. It's not only the trailing spaces but some files mix tabs and spaces.

Trailing spaces can be removed with a simple shell command even:

find . \( -name "*.[hm]" -or -name "*.mm" \) -exec sed -i '' -e 's/[[:space:]]*$//' {} \;

My concern is that there are a good few PRs open and doing something like this will inevitably cause merge conflicts, as this PR shows right now. So, I was going to wait until most PRs are merged... Not sure that'll be the case any time soon, so maybe after 3.8 I might clean up the spaces anyway. I'll have to touch a lot of files when changing the copyright banner anyway...

matrush commented 3 years ago

Apologies, this has been open for quite a while without a comment. In general I like the idea and I thought about cleaning up spaces for a while. It's not only the trailing spaces but some files mix tabs and spaces.

Trailing spaces can be removed with a simple shell command even:

find . \( -name "*.[hm]" -or -name "*.mm" \) -exec sed -i '' -e 's/[[:space:]]*$//' {} \;

My concern is that there are a good few PRs open and doing something like this will inevitably cause merge conflicts, as this PR shows right now. So, I was going to wait until most PRs are merged... Not sure that'll be the case any time soon, so maybe after 3.8 I might clean up the spaces anyway. I'll have to touch a lot of files when changing the copyright banner anyway...

Thanks! It's definitely not in a hurry, but would love to see it being merged at some point. This will make code style more consistent :)

Have you ever considered using clang-format to format this whole project?

dmaclach commented 3 years ago

I would certainly love to see clang-format run over the code. As the author of several of those PRs, I'm willing to do necessary rebasing/fixing.

On Tue, Jan 5, 2021 at 5:12 PM Chaoshuai Lü notifications@github.com wrote:

Apologies, this has been open for quite a while without a comment. In general I like the idea and I thought about cleaning up spaces for a while. It's not only the trailing spaces but some files mix tabs and spaces.

Trailing spaces can be removed with a simple shell command even:

find . ( -name ".[hm]" -or -name ".mm" ) -exec sed -i '' -e 's/[[:space:]]*$//' {} \;

My concern is that there are a good few PRs open and doing something like this will inevitably cause merge conflicts, as this PR shows right now. So, I was going to wait until most PRs are merged... Not sure that'll be the case any time soon, so maybe after 3.8 I might clean up the spaces anyway. I'll have to touch a lot of files when changing the copyright banner anyway...

Thanks! It's definitely not in a hurry, but would love to see it being merged at some point. This will make code style more consistent :)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/erikdoe/ocmock/pull/428#issuecomment-755007999, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACOFSMN744XB55D23KH6TLSYO2HVANCNFSM4NMUCJNQ .

erikdoe commented 3 years ago

It is done... I have not only fixed trailing whitespaces but also addressed tab/space issues and several obvious formatting inconsistencies. There is a clang-format config (thanks, Dave, for reminding me) but I decided against applying it to the entire source tree for two reasons:

  1. I couldn't get it to format the code in the OCMock "house-style", and
  2. there are a number of "harmless" inconsistencies that I didn't want fixed to limit noise.

Hope this is okay with you. Please have a look at the note in CONTRIBUTING.md, too.

dmaclach commented 3 years ago

Apparently my bluff has been called :)

Seriously though, a coding standard that can be machine applied is much appreciated.

I'll try and update my PRs as soon as I can. May be a day or two.

Cheers, Dave

On Thu, Jan 7, 2021 at 3:27 PM Erik Doernenburg notifications@github.com wrote:

It is done... I have not only fixed trailing whitespaces but also addressed tab/space issues and several obvious formatting inconsistencies. There is a clang-format config (thanks, Dave, for reminding me) but I decided against applying it to the entire source tree for two reasons:

  1. I couldn't get it to format the code in the OCMock "house-style", and
  2. there are a number of "harmless" inconsistencies that I didn't want fixed to limit noise.

Hope this is okay with you. Please have a look at the note in CONTRIBUTING.md, too.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/erikdoe/ocmock/pull/428#issuecomment-756448109, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACOFSIYUOLFNJUAVHCZWILSYY7NXANCNFSM4NMUCJNQ .

dmaclach commented 3 years ago

Hey Erik,

So, I misread your email apparently. I would much rather that we had a code format that we could just apply using clang-format even if it doesn't completely fit the current "house-style". As it is everytime I work on ocmock I get to reconfigure my editors so that they work in the ocmock style, and this is really painful. Being able to just run clang-format over my changes would be wonderful.

Can we work with the tooling instead of fighting it?

Cheers, Dave

On Thu, Jan 7, 2021 at 4:22 PM Dave MacLachlan dmaclach@gmail.com wrote:

Apparently my bluff has been called :)

Seriously though, a coding standard that can be machine applied is much appreciated.

I'll try and update my PRs as soon as I can. May be a day or two.

Cheers, Dave

On Thu, Jan 7, 2021 at 3:27 PM Erik Doernenburg notifications@github.com wrote:

It is done... I have not only fixed trailing whitespaces but also addressed tab/space issues and several obvious formatting inconsistencies. There is a clang-format config (thanks, Dave, for reminding me) but I decided against applying it to the entire source tree for two reasons:

  1. I couldn't get it to format the code in the OCMock "house-style", and
  2. there are a number of "harmless" inconsistencies that I didn't want fixed to limit noise.

Hope this is okay with you. Please have a look at the note in CONTRIBUTING.md, too.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/erikdoe/ocmock/pull/428#issuecomment-756448109, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACOFSIYUOLFNJUAVHCZWILSYY7NXANCNFSM4NMUCJNQ .

dmaclach commented 3 years ago

Hey Erik,

So I updated a bunch of my pull requests to get rid of the merge conflicts. I'll try and get the rest of them today if possible.

Doing so was rather painful trying to get styling right, and I'm sure I missed some stuff (I'll try and review yet again). I would love to spend my time improving OCMock rather than worrying about spaces and braces. I respect that you have a style that you like, and I'm sorry that the clang format tooling won't give you exactly what you want, but it would be really nice as an external developer to be able to just run clang-format over everything and not worry about the "house-style" and the "harmless" inconsistencies, that I would argue aren't harmless, and end up being a time suck.

Thanks again for all your work on this. BTW PR #470 is REALLY popular inhouse and has made our code significantly more readable.

Cheers, Dave

On Fri, Jan 8, 2021 at 9:40 AM Dave MacLachlan dmaclach@gmail.com wrote:

Hey Erik,

So, I misread your email apparently. I would much rather that we had a code format that we could just apply using clang-format even if it doesn't completely fit the current "house-style". As it is everytime I work on ocmock I get to reconfigure my editors so that they work in the ocmock style, and this is really painful. Being able to just run clang-format over my changes would be wonderful.

Can we work with the tooling instead of fighting it?

Cheers, Dave

On Thu, Jan 7, 2021 at 4:22 PM Dave MacLachlan dmaclach@gmail.com wrote:

Apparently my bluff has been called :)

Seriously though, a coding standard that can be machine applied is much appreciated.

I'll try and update my PRs as soon as I can. May be a day or two.

Cheers, Dave

On Thu, Jan 7, 2021 at 3:27 PM Erik Doernenburg notifications@github.com wrote:

It is done... I have not only fixed trailing whitespaces but also addressed tab/space issues and several obvious formatting inconsistencies. There is a clang-format config (thanks, Dave, for reminding me) but I decided against applying it to the entire source tree for two reasons:

  1. I couldn't get it to format the code in the OCMock "house-style", and
  2. there are a number of "harmless" inconsistencies that I didn't want fixed to limit noise.

Hope this is okay with you. Please have a look at the note in CONTRIBUTING.md, too.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/erikdoe/ocmock/pull/428#issuecomment-756448109, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACOFSIYUOLFNJUAVHCZWILSYY7NXANCNFSM4NMUCJNQ .