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

Remove extra whitespaces for whitespaces only lines #428

Closed
wants to merge 1 commit into from

Conversation

matrush
Copy link
Contributor

@matrush matrush commented May 28, 2020

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
Copy link
Contributor Author

matrush commented Jul 11, 2020

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

@erikdoe
Copy link
Owner

erikdoe commented Dec 20, 2020

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...

@erikdoe erikdoe added this to the OCMock 3.9 (or later) milestone Dec 20, 2020
@matrush
Copy link
Contributor Author

matrush commented Jan 6, 2021

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
Copy link
Contributor

dmaclach commented Jan 6, 2021 via email

erikdoe added a commit that referenced this pull request Jan 7, 2021
This supercedes #428. Please consider the remark added to CONTRIBUTING.md.
@erikdoe
Copy link
Owner

erikdoe commented Jan 7, 2021

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.

@erikdoe erikdoe closed this Jan 7, 2021
@dmaclach
Copy link
Contributor

dmaclach commented Jan 8, 2021 via email

@dmaclach
Copy link
Contributor

dmaclach commented Jan 8, 2021 via email

@matrush matrush deleted the removeExtraSpaces branch January 12, 2021 03:15
@dmaclach
Copy link
Contributor

dmaclach commented Feb 15, 2021 via email

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.

3 participants