Refactor identation and code style #27
Conversation
|
What tool did you use to format this? @marciodo perhaps we should add a |
|
I'm concerned that merging this pull request will potentially create a lot of conflicts with the other 2. Once we test everything, I think we could merge the other 2 pull requests and have this one rebased and resubmitted. @gustavosr8, Jeremy and I are interested in knowing more about the tool that you used. |
|
Hi @marciodo and @JJL772! I've made some adjustments to the .clang-format file you provided to better match the formatting style I've used previously, and I've included it in the commit. The advantage of using this file is that you can customize it to better suit your style preferences and integrate it into your CI pipeline. In order to apply the style to the files, you can use the following command:
It will find all the |
|
@gustavosr8, we are ready for this code-style pull request. Could you please see Jeremy's suggestions, apply your changes, and resubmit the pull request? See that we are basing all changes in the pre-release branch. |
|
Hey guys, sorry I took so long, I was on vacation. |
|
No problems, Gustavo! I believe we may need a few iterations until we get to a final result. I'm still beginning to look in detail at all the changes, so I'm not done with the review, yet. |
There was a problem hiding this comment.
Gustavo, I followed a link on the e-mail that doesn't belong to a branch in this repository. I don't know what happened: f7b99bb
All my comments are in this commit above.
Once you are ready, please apply your changes to the pre-release branch, not main.
| MCH_DEV_SUP_SET devStringinFru = { | ||
| 6, NULL, init_fru_stringin, init_fru_stringin_record, stringin_fru_ioint_info, read_fru_stringin, NULL}; |
There was a problem hiding this comment.
I think this is caused by the ColumnLimit. I'm thinking we should disable the ColumnLimit entirely by setting it to 0. @marciodo thoughts?
There was a problem hiding this comment.
The ColumLimit is useful, for example, in functions that have several parameters or parameters with several characters. In this case, clang-format will split the arguments into more lines, making the code easier to read.
But for the case above, it would be better if clang-format could do something like:
MCH_DEV_SUP_SET devStringinFru = {6, NULL, init_fru_stringin, init_fru_stringin_record,
stringin_fru_ioint_info, read_fru_stringin, NULL};
respecting the column limit.
There was a problem hiding this comment.
Hi guys, I couldn't find a way to achieve this. Do you have any idea how to do it?
There was a problem hiding this comment.
Maybe this is not possible. I'll try a few things, although I'm not sure when.
|
There are conflicts in 4 files. I believe that you applied clang-format in the main branch, not pre-release, and when you modified the base branch for the pull request the conflicts showed up. Would you mind running clang-format in the pre-release branch instead of main? |
In the current code style, there are some misleading indentation, which makes the code a little hard to understand, so I fixed some of this mistakes.
Changed the function headers to have the return type in the same line as the function name, and add a tab before the local variable declarations . Change the indentation style to use four spaces, and change the space usages in function calls, if-else statements, switch cases, and other specific situations.
I guess some of this code styles was a project choice, so it can be removed from this commit if you think it should be the way that it is!