ArduPilot / ardupilot

ArduPlane, ArduCopter, ArduRover, ArduSub source
http://ardupilot.org/
GNU General Public License v3.0
11.08k stars 17.66k forks source link

Run pre-commit for ArduPilot source automatically #24555

Open Ryanf55 opened 1 year ago

Ryanf55 commented 1 year ago

AP_DDS requires pre-commit to be run. If you don't have it set up, you have to run it and squash the changes. CI could actually support changing the files automatically.

Last time we discussed this in dev call for how astyle isn't applied automatically, I suggested pre-commit, however that was shot down because astyle is not available on all developer machines. With pre-commit auto-fixing the files in CI, this is no longer a concern, and could be adopted more widely across the repo.

          YES!!!! Except it's really a global thing that doesn't belong inside AP_DDS. Ideally we'd have some suggestions to use tools like these when creating a PR.... as I suggested to @peterbarker during his tools talk at the recent Dev conference.

https://youtu.be/-5JoR29ZlrA?t=1738

Originally posted by @magicrub in https://github.com/ArduPilot/ardupilot/issues/23535#issuecomment-1512330756

Ryanf55 commented 1 year ago

Looks theres' some conflicts with Tools/gittools/pre-commit.py which overwrites pre-commit.

(venv-ardupilot) ryan@ryan-B650-970:~/Development/ardu_ws/src/ardupilot$ pre-commit install
Running in migration mode with existing hooks at .git/hooks/pre-commit.legacy
Use -f to use only pre-commit.
pre-commit installed at .git/hooks/pre-commit

Additionally, pre-commit doesn't run without errors right now. It's not enforced in CI, so there will be regressions.

To start, we can add exceptions for all the current regressions, and enforce no new regressions in CI.

Ryanf55 commented 1 year ago

Would be nice to run astyle in CI too to prevent errors like these: https://github.com/ArduPilot/ardupilot/pull/24792#discussion_r1307250568

Ryanf55 commented 2 weeks ago

Asks from Tridge:

Random - we want force push to remove merge on CI pass unless you have merge rights. The commit per subsystem needs to do more than just the head. The python git API's could do it.

khancyr commented 1 week ago
Ryanf55 commented 1 week ago
  • We don't expect people to format partial files [ ] that could cause issue on start as precommit will try to fix modified files as people are making change

pre-commit only runs when you commit, which means you are done making changes and have typed git commit.

Ryanf55 commented 1 week ago
  • We need a way to ignore it [ X ] We just need to document it, it is easy to disable and force no check on cmdline, not sure on IDE

I need to know how people use git in an IDE without CLI. Can anyone share what tools they use or that new contributors use that aren't CLI?

timtuxworth commented 1 week ago

Just go with whatever VSCode does. I use CLI, but I think most IDE people are using VSCode.

Ryanf55 commented 1 week ago

Just go with whatever VSCode does. I use CLI, but I think most IDE people are using VSCode.

There is nothing built in to the default vscode that handles C++ formatting. It's all through extensions.