Coding style and static analysis #38
Comments
|
For Python it is also a good idea to use a tool that checks Python3 compatibility as well. The IDE http://ninja-ide.org/ does that. There might be a command line tool behind this as well. |
|
I'm all in favor of requiring consistent coding style. |
|
I think encourage is more practical the require. |
|
@c-morley, agreed, encouragement is more realistic than requirement, and @Strahlex, some of those tools could be used to help automate stylistic issues to make review easier for Maintainers. @c-morley, I don't quite understand about rejecting a 'project'; what does 'project' mean here? IMO, external projects like Classicladder need to be unbundled from MK if possible (I realize that's not immediately possible in this case) to comply with DFSG and other distro packaging guidelines, and GUIs need to be spun out for other reasons (there's another ticket addressing this). In any case, it's fine with me to allow different styles within different projects (and IMO that's another indicator a project should be spun out!). |
|
@c-morley: Sounds like those are candidates for spinning out (git subtree, build system, packaging) of the core Machinekit code? @luminize probably knows more than most anyone else in the MK community about git subtrees. I wonder if that wouldn't make the lives of folk like you easier too, if there were one GitHub repo that could be built against either of MK or LCNC projects. |
|
Is there any tool that cleans up tabs and spaces? I think it makes sense to check for such problems before merging a patch. Most editors do not display tabs and spaces differently and thats really annoying. I would not force a specific coding style on anyone, but providing a standard astyle file for C,C++ and Python would make sense. Especially for those who do not program frequently and need some guidelines anyway. Static analysis is whole different thing and should be enforced if possible. Well, it can be annoying and too strict (MISRA C), but it could prevent many ugly bugs introduced in projects. |
|
yes: Emacs indent-region. Please use the settings for C++ (put in your .emacs): (setq c-default-style If you re-indent a file, please make it a separate commit - do not mix editing with a whitespace-only change, which will make it harder to resolve merge conflicts. |
|
I recently created a PR to the Qt project and came across a high sophisticated sanity checking system. Something like this could be useful for us too: https://wiki.qt.io/Early_Warning_System |
|
E.g. the use to sanitize commit messages and do some basic checks like indentation: https://github.com/qtproject/qtrepotools/tree/master/git-hooks |
|
Having a check for code-style in CI could have some benefits. Could we use the qtrepotools somehow to reject PRs that do not adhere to the style guide? Otherwise: do you know which tools we'd need for this? Also, to adhere to the style guide, the current code needs to be cleaned up. What is the best approach for this? One large commit at some point? |
|
Can we generate a list where stuff is bad? parallellism and small bites are the key IMO. |
|
@luminize I think a lot can be done automatically, like tabs/spaces, but I'm not sure whether that's a good idea for the current code-base. |
|
@bobvanderlinden I'd love to integrate a coding style check into CI. The main concern I'm aware of for mass cleanups is making it hard to perform merges. If we did do something like this, it should definitely be limited to the Machinekit core code (RTAPI + HAL), which just a small number of people contribute to. Mass cleanups should be kept out of the CNC application code, since we're preparing to spin it out and leave maintenance for other projects. Any changes to that code to work with MK core should be easy to pick off into other LCNC forks. |
|
There is GitHub extension for doing automatic code analysis for pull requests: https://github.com/integrations/code-climate |
|
@Strahlex Code Climate doesn't seem to support C/C++ (yet?). It does do Python though, so maybe Travis and Code Climate could be combined? It could also be very usable for related Python and JS projects too. |
|
Might be useful too: http://editorconfig.org/ |
|
Just have seen |
|
Frama-C acts both as static code analysis tool and as theorem prover if required. Could be useful for checking C HAL components e.g. |
|
I just added Machinekit to Coverity Scan: https://scan.coverity.com/projects/machinekit-machinekit Let's see if it proves useful. |
|
Interesting. |
|
@luminize In a lot of cases we are not dealing with user input, so not checking the data size might be safe. However, I cannot think of any good reason why we shouldn't write the copy functions in a safe way as it is basically for free to do so. |
|
Using editorconfig is pretty easy to achieve: https://github.com/qtquickvcp/QtQuickVcp/blob/master/.editorconfig |
|
Yeah, i'm using it in one of my own projects. Let's put one in the main MK repo for the languages in use, and people who use editorconfig will be served. Let's make it optional to use it, and not mandatory. Then we can close the coding style part. |
ArcEye
added
the
Cross-Stacks Issue
|
Yo, what's the status on this? I can't find any editor config files anywere, and I've noticed that indentation is really inconsistent. For now I'll follow the CodingStyle file in src |
|
@MicroTransactionsMatterToo , Personally, I was looking into creating post pull hook which would amend every commit in the request with code style enforcement and then run it by other linter functions and static analysis. Problem is - it creates new hashes, so that one creates a bit of burden. But not much, given that the need to rebase the code onto Machinekit-* master will still be there. That way, the code would be updated to new styles piece by piece and not in Big Bang. CodingStyle is nice up to the point the editor ignores it. (So I am proponent of forceful solution.) |
|
@cerna Right. I'll do that. EditConfig only provides for a few formatting options, so I might just write a few different config files for different editors in my free time |
|
@cerna Right, I've taken a look and clang-format seems the best option, since it's pretty likely to be installed on most linux distros. I can write either a pre-commit hook or post pull hook. There's an option now to version control git hooks so a pre-commit hook would apply the given formatting rules to any changed/new files at commit without people having to re-add the hook, but I dunno whether that'd be ideal. What are your thoughts? Another option is GNU indent, but that's it's own package, where as clang-format comes with clang by default |
|
@MicroTransactionsMatterToo , I am not exactly expert on git hooks (understanding of the century). So let me get it straight and sum it up. So far, the workflow is one forks the machinekit/Machinekit-* repository on GitHub to user/Machinekit-*, then clones the user/Machinekit-* locally into /Machinekit-* on which the development is actually done. You would create Client-Side pre-commit git hook on the /Machinekit-* repository to enforce the code formatting, right? Something like this. So every developer would have to have a Clang on his machine. However, the last time I checked, the git hooks were not version controlled and did not propagate from remote->local repository, so
from this I take it is no longer such? Nowadays, git hooks in remote repository do propagate when cloning or fetching/pulling and are version controlled? Because that would be great! The post-pull git hook would be Server-side one on machinekit/Machinekit-* repository, right? And do you mean that it would only check the formatting of proposed pull request or it should do some changes to commits? Thanks. |
|
@cerna Git has added a mechanism for version controlling commit hooks. You can set a git hook repository explicitly that is outside of |
|
But yeah, basically as you described
|
Right, I am just making sure we are on the same page.
OK, truth be told, everything is better than how it is now. And I (at least personally) am proponent of not-perfect solutions which moves us few steps towards perfect. |
|
Looked into the git hooks a little bit more and it looks like the automatic hook propagation from the remote to local repository on So potential developer would have to something akin to: Which is not that bad, but you have to know about it and it is one step more. (My general experience is that users don't read and as such many will not do this.) Reading githooks.com I discovered that this is probably common problem and there exist many libraries, frameworks and projects for git hook management, some even with CI (Travis) support and CMake support. Given that some looks like very easy to set up and include pre-made formaters (its popular option), maybe its viable option? (Would it help in any way with build process, @zultron ?) And OK, having one script doing it all server-side on main Machinekit repository was idiotic idea. Industry standard is to have main client-side pre-commit script which will do the heavy-lifting and then second one server-side, which will test every pull request and reject those which fail. That how we should do it. |
|
@cerna Looks like you're right, which kinda makes the core.hooksPath useless outside of being able to keep the hook scripts in the version control. I think it'd be wise to keep extra tools out of the repo. Once @kinsamanka is done implementing CMake into both repositories we can look into fully integrating it into the build process, which we could either use to set the |
Given the fact that we want to have machinekit-hal and machinekit-cnc (and in future maybe some other repository), you are probably right. But we want to have some files/settings coupled with the repository: the
Well, you use the Travis for the Continuous Integration and the fact, that it is capable of running tasks on your behalf. You have scripts and settings. If you use these scripts as a pre-commits hook handlers on the local repository side to perform some work, you can use the exact same scripts to perform tests on other machines that the work was already done. Given the common inputs, it should reach the same outputs. So we could use the formatting as a test on the CI: If the formatting script does nothing, then the work was already done in pre-commit stage and the test pass, otherwise the formatting is wrong and the pull request should fail and @luminize will (by the C4 rules) know not to merge. |
|
And to move forward, lets talk about the formatting. This is the one I like: .clang-format. Comments? Opposition? |
|
@cerna I've implemented the hooks and the clang format stuff. It only implements the formatting stuff, so naming and code style stuff isn't enforced (Couldn't find any decent tool to enforce this stuff that wouldn't require writing our own checks). Should I open it as a pull request or something? |
Great.
That is OK, I think. Small steps. I would rather have code formatting in place for all used languages first. (Python and shell scripting.) Based on how it works, then we can start war on how to name symbols.
Yes. Pull request. |
Tue Dec 2 10:10:02 2014
Originally opened as machinekit/machinekit#391
We should enforce a specific coding style for every language in the project. This can e.g. be done by using astyle. My code editor (Kate) always shows mixed indentations (tabs and spaces) everywhere. This makes the code unreadable on some platforms and is possible error source.
Furthermore every piece of code that is commit should be checked with suitable static code analysis tool. For C and C++ cppcheck and clang might be suitable. For Python pep8 is the way to go. This ensures that common error will not make its way into the project.
The text was updated successfully, but these errors were encountered: