-
Notifications
You must be signed in to change notification settings - Fork 820
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
Expand tab to spaces #3210
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Have we confirmed that the feature to ignore certain revisions in GitHub blame works? I think this is a must.
- IMO the PR also needs to be more clear about what the official indentation style from now on
- Also add instruction in the PR about how to ignore whitespace while seeing PRs/diffs
- Maybe also add an .editorconfig file, as a best effort solution for future edits (and add instructions about required plugins)
+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.) |
The alternatives would be:
I tested both and they worked.
If we want to continue using the current style, it should be using space with size 4.
Added on the PR description.
Let me do some tests for this |
insert_final_newline = true | ||
indent_style = space | ||
indent_size = 4 | ||
trim_trailing_whitespace = true |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
There was a problem hiding this 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.
Move to #3292 |
This is a followup for pjsip#3292 and pjsip#3210 but regarding aconfigure.ac
This is a followup for pjsip#3292 and pjsip#3210 but regarding aconfigure.ac
Currently, the source code was written with these formatting setting:
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:
Notes:
git blame -w
to find the commit where the code was actually written.