Add black formatter #392

tylerjw · 2021-03-20T20:53:50Z

Description

This adds the black python formatter to CI. This formatter is like clang-format in that it will automatically reformat all code to match a pre-defined style. This makes reading the code much nicer. It will also enforce that all the python code is python3.

I know this is a large reformatting PR. I will make the same PR against the ros1 moveit repo to add the same functionality there for this and the other pre-commit tests. That way this will not impact being able to sync between the two repos.

codecov · 2021-03-20T21:55:08Z

Codecov Report

Merging #392 (c10e52a) into main (cc557b4) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #392      +/-   ##
==========================================
+ Coverage   53.33%   53.33%   +0.01%     
==========================================
  Files         210      210              
  Lines       18814    18814              
==========================================
+ Hits        10032    10033       +1     
+ Misses       8782     8781       -1

Impacted Files	Coverage Δ
...raint_samplers/src/default_constraint_samplers.cpp	`77.54% <0.00%> (-0.36%)`	⬇️
...meterization/work_space/pose_model_state_space.cpp	`84.56% <0.00%> (+1.48%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc557b4...c10e52a. Read the comment docs.

felixvd

I just had a quick scan through this. Looks good overall though.

Could you check if there's a setting to delete the trailing comma in a list of arguments? I think that's just atrocious.

felixvd · 2021-03-22T08:20:32Z

moveit_commander/bin/moveit_commander_cmdline.py

+        action="store_true",
+        dest="interactive",
+        default=True,
+        help="Run the command processing script in interactive mode (default)",


I didn't know ending a list of arguments with an open comma like this wasn't a syntax error. I guess it's just hideous. The more you know.

There's no setting for this in black, right?

there is a good reason for this: it avoids syntax errors when (named) arguments get shuffled around -- and the person doing that forgets to add the comma and then spends I-don't-know-how-long figuring out where the syntax error is.

I guess. Although when I see a comma followed by a closed bracket I look like this:

I also don't know how often people reorder arguments vs how often code is read, but it doesn't matter. It's the setting in black and I don't have strong objections.

felixvd · 2021-03-22T08:24:44Z

moveit_commander/test/python_time_parameterization.py

-        self.assertTrue(len(retimed_plan.joint_trajectory.points) > 0, "Retimed plan is invalid")
-        retimed_plan = self.time_parameterization(plan, "time_optimal_trajectory_generation")
-        self.assertTrue(len(retimed_plan.joint_trajectory.points) > 0, "Retimed plan is invalid")
+        retimed_plan = self.time_parameterization(


Kind of a lot of lines, but it's not critical.

henningkayser

Really nice! Comments are mostly nits, maybe the config could be tweaked a little

henningkayser · 2021-03-22T18:46:23Z

moveit_ros/planning_interface/test/robot_state_update.py

@@ -33,7 +37,9 @@ def test(self):
            error_code1, plan, time = self.plan(target)
            error_code = MoveItErrorCodes()
            error_code.deserialize(error_code1)
-            if (error_code.val == MoveItErrorCodes.SUCCESS) and self.group.execute(plan):
+            if (error_code.val == MoveItErrorCodes.SUCCESS) and self.group.execute(


This seems a bit out of place

henningkayser · 2021-03-22T18:47:14Z

moveit/scripts/create_readme_table.py

-                line += ' | '
-                print('  {}: {} {}'.format(ros, response, params['url']), file=sys.stderr)
+                line += " | "
+                print(


also a bit odd

henningkayser · 2021-03-22T18:48:09Z

moveit_commander/src/moveit_commander/planning_scene_interface.py

@@ -119,7 +129,9 @@ def add_plane(self, name, pose, normal=(0, 0, 1), offset=0):
        co.plane_poses = [pose.pose]
        self.__submit(co, attach=False)

-    def attach_mesh(self, link, name, pose=None, filename='', size=(1, 1, 1), touch_links=[]):
+    def attach_mesh(


should we change the max time length?

henningkayser · 2021-03-22T18:49:50Z

moveit_ros/planning_interface/test/robot_state_update.py

@@ -6,7 +6,9 @@
 import rostest
 import os

-from moveit_ros_planning_interface._moveit_move_group_interface import MoveGroupInterface
+from moveit_ros_planning_interface._moveit_move_group_interface import (


not sure, this also seems like a weird rule for a single import.

rhaschke · 2021-03-22T19:27:56Z

black - by design - "is not configurable". I think the only option is --line-length. It's configured like this:

  - repo: https://github.com/psf/black
    rev: 20.8b1
    hooks:
      - id: black
        args: ["--line-length", "100"]

tylerjw · 2021-03-22T19:35:22Z

black - by design - "is not configurable". I think the only option is --line-length. It's configured like this:
  - repo: https://github.com/psf/black
    rev: 20.8b1
    hooks:
      - id: black
        args: ["--line-length", "100"]

@henningkayser imo we should just take black the way it is. The line length setting included. One thing that's great about black is if you install pre-commit locally you can just stop trying to edit the formatting of your python code and it'll do it for you and we'll all have a nice consistent style. I honestly wish clang-format only had --style options and not a config file.

felixvd

LGTM even with the idiosyncrasies.

tylerjw requested review from henningkayser and rhaschke March 20, 2021 20:53

tylerjw mentioned this pull request Mar 20, 2021

Add pre-commit with GitHub Action moveit/moveit#2557

Closed

tylerjw added this to In progress in Sprint 00 via automation Mar 20, 2021

felixvd reviewed Mar 22, 2021

View reviewed changes

henningkayser approved these changes Mar 22, 2021

View reviewed changes

Sprint 00 automation moved this from In progress to Reviewer approved Mar 22, 2021

tylerjw added 2 commits March 22, 2021 15:13

Add black formatter

b604c2a

Fix black formatting errors

c10e52a

tylerjw force-pushed the add-black-formatter branch from 5b0a690 to c10e52a Compare March 22, 2021 21:14

felixvd approved these changes Mar 23, 2021

View reviewed changes

henningkayser merged commit 6b2ad23 into moveit:main Mar 23, 2021

Sprint 00 automation moved this from Reviewer approved to Done Mar 23, 2021

tylerjw deleted the add-black-formatter branch March 23, 2021 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add black formatter #392

Add black formatter #392

tylerjw commented Mar 20, 2021

codecov bot commented Mar 20, 2021 •

edited

Loading

felixvd left a comment

felixvd Mar 22, 2021

gavanderhoorn Mar 22, 2021

felixvd Mar 23, 2021

felixvd Mar 22, 2021

henningkayser left a comment

henningkayser Mar 22, 2021

henningkayser Mar 22, 2021

henningkayser Mar 22, 2021

henningkayser Mar 22, 2021

rhaschke commented Mar 22, 2021

tylerjw commented Mar 22, 2021

felixvd left a comment

Add black formatter #392

Add black formatter #392

Conversation

tylerjw commented Mar 20, 2021

Description

codecov bot commented Mar 20, 2021 • edited Loading

Codecov Report

felixvd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henningkayser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhaschke commented Mar 22, 2021

tylerjw commented Mar 22, 2021

felixvd left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 20, 2021 •

edited

Loading