[Bug] FreeCAD Project Policy on code formatting is confusing #10200
Replies: 17 comments
-
|
I've been using Black for the Addon Manager and, while I don't love all of its formatting choices, I have to say it's a delight to not have to think (at all) about formatting. For C++, Visual Studio is happy to use a clang-format file, so I also don't feel strongly about exactly what formatting options we choose there. In general I prefer to let the computer do the formatting so I can focus on the code. I'd be perfectly in favor of doing one huge automated reformat pass, adding those commits to the .git-blame-ignore file, and moving on with life. The only thing I hate is inconsistent formatting -- if we set a standard, then in my opinion at least entire files have to be converted in one go, I don't think we should do it one function (or worse, one line!) at a time. |
Beta Was this translation helpful? Give feedback.
-
|
Is there a downside to having automatic formatting applied after commit? |
Beta Was this translation helpful? Give feedback.
-
|
If the commit is signed (and basically all of mine are), then that needs to be a separate commit. IMO it would be better to have a required pre-commit hook -- the Qt project does that, for example. Actually maybe OCC does too, it's been a while. |
Beta Was this translation helpful? Give feedback.
-
|
There is a forum discussion here about clang-format. Some progress was made. |
Beta Was this translation helpful? Give feedback.
-
|
I think there is one important decision to be made: Is a descriptive text a prerequisite to implementing formatting? I suggest no, but there does need to be more discussion to achieve consensus. That discussion is itself useful documentation. |
Beta Was this translation helpful? Give feedback.
-
|
Are we talking about a "style guide" such as https://google.github.io/styleguide/cppguide.html Or just a couple of rules, such as: my inclination would be to start with just a pair of rules - 1 for python and 1 for c++. The style guide seems like too big of a struggle right now. |
Beta Was this translation helpful? Give feedback.
-
|
A style guide would be the specific rules for how code should be formatted. We could start with the simple rules you describe. For Python, I'd be happy if we said "PEP8 and Black" and left it at that. The policy is the part that needs at least a little discussion. What do we do about existing code when a minor change is made? Do we pursue style-guide compliance on new code and refactored code only or does it apply the next time any file is touched for any reason? I'm guessing that the second one would be very unpopular as long as we're looking at large re-basing efforts around toponaming. |
Beta Was this translation helpful? Give feedback.
-
|
"style guide" can be any human readable document stating format rules. It is not necessarily the reference that an enforcement mechanism is required to comply with. A "policy" in this case is more correctly like the decision to require auto formatting and perhaps to define the required mechanism. For C++ clang-format is a human-readable statement of rules designed to be parsable by an enforcement mechanism. It is explicit, and any change therein has immediate impact. A descriptive text e.g. a wiki entry, on the other hand, is not explicit and changes therein not only have no direct impact but can easily be out of step with a reference document. So I agree with @WandererFan that the reference document for C++ in FC be the clang-format file in the project, and that a separate explanatory style guide be left for another time. As an aside, the clang-format on FC is based on LLVM. There are many base styles out there. For python, Black seems to be popular. It is self-described as a strict subset of PEP8. So whilst Black does have a descriptive text, there is not an accessible or changeable ruleset. Looks like "take it or leave it", and they say so. |
Beta Was this translation helpful? Give feedback.
-
"New code MUST be formatted according to the appropriate language standard." The second rule provides an out for fixing typos, etc. Formatting the whole file is no big deal for C++ and Qt Creator as there is an option to reformat on save. Don't know if it is an extra step for Python or other IDE. |
Beta Was this translation helpful? Give feedback.
-
|
I'd also suggest that if an existing file is being reformatted because it's being touched for some other reason that the actual changes and the formatting changes be in different commits. Makes it easier to review |
Beta Was this translation helpful? Give feedback.
-
|
I very much agree -- we can also then add that commit to the .git-blame-ignore-revs file, so it doesn't pollute the history in git blame. |
Beta Was this translation helpful? Give feedback.
-
Bearing in mind that such extra rule becomes obsolete once a global reformat is performed. Assuming unified formatting of course. |
Beta Was this translation helpful? Give feedback.
-
|
What is the next step? A draft PR of a policy? |
Beta Was this translation helpful? Give feedback.
-
|
Yes. That would be ideal. Want to take a stab at it? |
Beta Was this translation helpful? Give feedback.
-
|
See #8232. It appears there are problems with clang-format in some IDEs. There are also issues that various versions of clang support more or fewer formatting instructions. It looks like asking Contributors to format their C++ code locally is not going to fly. I think we need to install a hook in the CI process to do the reformatting with a standard version of clang-format. We should also consider a one-shot reformatting pass on the repo. |
Beta Was this translation helpful? Give feedback.
-
|
Related to #9140 |
Beta Was this translation helpful? Give feedback.
-
|
We do now have clang-format pre-commit hooks enabled in some sections of the repository, and developers should move towards having pre-commit installed and enabled on their FreeCAD repositories. Individual Workbench Maintainers should work with their development teams to develop a strategy on when and how to apply that precommit hook to their Mod subdirectory, and to add those commits to the .git-blame-ignore-revs file. I am happy to assist with any technical concerns about how this process works. |
Beta Was this translation helpful? Give feedback.
-
Is there an existing issue for this?
Forums discussion
N/A
Version
0.21 (Development)
Full version info
Subproject(s) affected?
No response
Issue description
Most developers agree that having a project-wide standard for code formatting is useful. It makes reading and reviewing code easier and reduces errors. However, there isn't a clear consensus on what the FreeCAD project standard is or how to apply it.
For example:
Should python adhere to PEP8 strictly or allow for different line lengths?
Should python files be run through Black before committing?
Do published formatting standards only apply to new contributions or should older code be reformatted the next time it is touched?
Can individual workbenches vary/extend the project-wide standard?
This issue is created to start a discussion around code formatting with the goal of eventually publishing a guideline.
Anything else?
No response
Code of Conduct
Beta Was this translation helpful? Give feedback.
All reactions