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

Expand tab to spaces #3210

Closed
wants to merge 3 commits into from
Closed

Expand tab to spaces #3210

wants to merge 3 commits into from

Conversation

trengginas
Copy link
Member

@trengginas trengginas commented Aug 9, 2022

Currently, the source code was written with these formatting setting:

Indentation: 4
Tab size: 8

Some editors might not be able to correctly format when displaying/editing the code, resulting miss-alignment.
This PR will replace the Tab character with spaces to address such issue.
This PR will also remove svn keyword $Id$ string which is no longer used.

For conformity, a .editorconfig file is included on this PR. Please check https://editorconfig.org/ for the editor+plugins that supports it. Basically it is using indentation style with space size 4.
Here are some editor tested:

  • Eclipse (not working)
  • VS Code (1.70.2) + EditorConfig for VS Code plugin
  • Visual Studio 2022

Notes:

  • Refer to post on how to ignore whitespaces when viewing file changes on PR.
  • To ignore the whitespaces-only change from this PR, use git blame -w to find the commit where the code was actually written.

Copy link
Member

@bennylp bennylp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Have we confirmed that the feature to ignore certain revisions in GitHub blame works? I think this is a must.
  2. IMO the PR also needs to be more clear about what the official indentation style from now on
  3. Also add instruction in the PR about how to ignore whitespace while seeing PRs/diffs
  4. Maybe also add an .editorconfig file, as a best effort solution for future edits (and add instructions about required plugins)

@wosrediinanatour
Copy link
Contributor

wosrediinanatour commented Aug 9, 2022

+1 for .editorconfig, which also contains charset, end_of_line, trim_trailing_whitespace, insert_final_newline etc.

@bennylp
Copy link
Member

bennylp commented Aug 9, 2022

+1 for .editorconfig, which also contains charset, end_of_line, trim_trailing_whitespace, insert_final_newline etc.

They're not available on all platforms/plugins though (e.g. eclipse)

@wosrediinanatour
Copy link
Contributor

+1 for .editorconfig, which also contains charset, end_of_line, trim_trailing_whitespace, insert_final_newline etc.

They're not available on all platforms/plugins though (e.g. eclipse)

yes, but at least it is a documentation you can refer to and the e.g. Makefile or Python code styles could be part of it (in case of C and C++, .clang-format file would make it for some people even better, since you could define the coding style more or less completely, you could format files by clang-format, and, hence, contributing would be easier.)

@trengginas
Copy link
Member Author

  1. Have we confirmed that the feature to ignore certain revisions in GitHub blame works? I think this is a must.

The alternatives would be:

  • adding .git-blame-ignore-revs files which include the ignored commit.
  • use git blame -w which will ignore whitespace changes. (no need to add .git-blame-ignore-revs file)

I tested both and they worked.

  1. IMO the PR also needs to be more clear about what the official indentation style from now on

If we want to continue using the current style, it should be using space with size 4.

  1. Also add instruction in the PR about how to ignore whitespace while seeing PRs/diffs

Added on the PR description.

  1. Maybe also add an .editorconfig file, as a best effort solution for future edits (and add instructions about required plugins)

Let me do some tests for this

insert_final_newline = true
indent_style = space
indent_size = 4
trim_trailing_whitespace = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this config won't work with VS Code? (see internal discussion email)

Also, perhaps add note in the PR what editors you have tested with and which work and which doesn't, in terms of viewing and adding/editing code, so users know what to expect.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work after the tab is expanded to spaces. It's not going to work if spaces is still use in the code.

Copy link
Member

@sauwming sauwming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay to me. But let's wait close to 2.13 release so we can merge with the latest master first.

@sauwming sauwming mentioned this pull request Sep 29, 2022
Copy link
Member

@sauwming sauwming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 2.13, I think we can integrate this.

Remember to merge the master first and then expand the indentation of the latest merge.

Then after this PR is merged, create another PR to ignore this commit from blame.

@trengginas
Copy link
Member Author

Move to #3292

@trengginas trengginas closed this Nov 22, 2022
@trengginas trengginas deleted the expand-tab-to-spaces branch November 22, 2022 09:27
@sauwming sauwming mentioned this pull request Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants