search

moveit / moveit_resources

URDFs, meshes, and config packages for MoveIt testing
61 stars 115 forks source link

Only run pre-commit if push is to galactic branch #145

Closed vatanaksoytezer closed 2 years ago

rhaschke commented 2 years ago

Why would you want to restrict such a trivial check to the main branch only? If contributors commit to their fork, they should see issues as well.

simonschmeisser commented 2 years ago

I wondered the same, there seems to be some reason as you also merged #144 in the same vein. Please provide at least a tiny hint to your motivation/reasoning in PR's in future

vatanaksoytezer commented 2 years ago

It was redundantly running on every PR twice, because it is push and a pull request at the same time. Contributors can already run locally and it would still run in every PR. Every other repo is already configured that way in ros planning org including moveit. I thought it was very obvious and easy fix to redundant runs.

rhaschke commented 2 years ago

It was redundantly running on every PR twice

Not really. If you push to a private fork first, only the push trigger is activated. If you then create a PR, the PR trigger is activated in ros-planning. Only if you push to a branch in ros-planning directly and create a PR, both triggers are activated simultaneously.

By the way, MoveIt1 applies this check for any PR and push commit :wink:

vatanaksoytezer commented 2 years ago

I don't mind reverting it, if it is a very desired behaviour by other maintainers but I don't see it bringing anything useful at all. pre-commit is meant to be run before your commit as a git hook anyway and running it in pr is already enough in my opinion. The behaviour you described is still redundant. Say you pushed in your private fork and opened a PR. This action will run once in your fork and once in the PR. I would argue it should not be that way as well in moveit1.

rhaschke commented 2 years ago

Not all contributors have the pre-commit hook installed locally (unfortunately). Hence, having (early) feedback about code formatting quality is still helpful for them in my opinion. This hopefully can save us some extra runs of the full, much more costly CI tests within a PR, because contributors will cleanup their code before and not after making the PR. We shouldn't discuss this in more detail. I don't care very much about this detail. But - as the formatting test is rather fast - I would prefer to keep it.