Address minor feedback comments for ros2bag

Signed-off-by: Christophe Bedard <[email protected]>

ros2bag/ros2bag/api/__init__.py

@@ -158,37 +158,44 @@ def check_not_negative_int(arg: str) -> int:
         raise ArgumentTypeError('{} is not the valid type (int)'.format(value))
 
 
-def add_standard_reader_args(parser: ArgumentParser, multiple_readers: bool = False) -> None:
+def add_standard_reader_args(parser: ArgumentParser) -> None:
     """
-    Add arguments for one of multiple input bags.
+    Add arguments for one input bag.
 
     :param parser: the parser
-    :param multiple_readers: whether this is part of a verb that takes in multiple input bags
     """
-    # If multiple readers, let user provide an input bag path using an optional positional arg, but
-    # require them to use --input to provide an input bag with a specific storage ID
+    reader_choices = rosbag2_py.get_registered_readers()
+    parser.add_argument(
+        'bag_path',
+        nargs=None,
+        type=check_path_exists,
+        help='Bag to open.')
+    parser.add_argument(
+        '-s', '--storage', default='', choices=reader_choices,
+        help='Storage implementation of bag. '
+             'By default attempts to detect automatically - use this argument to override.')
+
+
+def add_standard_multi_reader_args(parser: ArgumentParser) -> None:
+    """
+    Add arguments for multiple input bags.
 
+    :param parser: the parser
+    """
+    # Let user provide an input bag path using an optional positional arg, but require them to use
+    # --input to provide an input bag with a specific storage ID
     reader_choices = rosbag2_py.get_registered_readers()
-    bag_help_multiple_readers_str = ''
-    storage_help_multiple_readers_str = ''
-    if multiple_readers:
-        bag_help_multiple_readers_str = \
-            ' Use --input to provide an input bag with a specific storage ID.'
-        storage_help_multiple_readers_str = \
-            ' (deprecated: use --input to provide an input bag with a specific storage ID)'
     parser.add_argument(
         'bag_path',
-        nargs='?' if multiple_readers else None,
+        nargs='?',
         type=check_path_exists,
-        help='Bag to open.' + bag_help_multiple_readers_str)
+        help='Bag to open. Use --input instead to provide an input bag with a specific storage ID.')
     parser.add_argument(
         '-s', '--storage', default='', choices=reader_choices,
         help='Storage implementation of bag. '
              'By default attempts to detect automatically - use this argument to override.'
-             + storage_help_multiple_readers_str)
-
-    if multiple_readers:
-        add_multi_bag_input_arg(parser, required=False)
+             ' (deprecated: use --input to provide an input bag with a specific storage ID)')
+    add_multi_bag_input_arg(parser, required=False)
 
 
 def add_multi_bag_input_arg(parser: ArgumentParser, required: bool = False) -> None:

ros2bag/ros2bag/verb/play.py

@@ -15,7 +15,7 @@
 from argparse import FileType
 
 from rclpy.qos import InvalidQoSProfileException
-from ros2bag.api import add_standard_reader_args
+from ros2bag.api import add_standard_multi_reader_args
 from ros2bag.api import check_not_negative_float
 from ros2bag.api import check_not_negative_int
 from ros2bag.api import check_positive_float
@@ -44,7 +44,7 @@ class PlayVerb(VerbExtension):
     """Play back ROS data from a bag."""
 
     def add_arguments(self, parser, cli_name):  # noqa: D102
-        add_standard_reader_args(parser, multiple_readers=True)
+        add_standard_multi_reader_args(parser)
         parser.add_argument(
             '--read-ahead-queue-size', type=int, default=1000,
             help='size of message queue rosbag tries to hold in memory to help deterministic '
@@ -198,6 +198,11 @@ def main(self, *, args):  # noqa: D102
             topic_remapping.append('--remap')
             topic_remapping.append(remap_rule)
 
+        # Do not allow using both positional arg and --input option for input bags
+        if args.bag_path and args.input:
+            return print_error(
+                'do not mix the [bag_path] positional argument and the -i,--input option; '
+                'for multiple input bags, use -i,--input multiple times')
         # Add bag from optional positional arg, then bag(s) from optional flag
         storage_options = []
         if args.bag_path:
@@ -207,7 +212,7 @@ def main(self, *, args):  # noqa: D102
                 storage_config_uri=storage_config_file,
             ))
             if args.storage:
-                print(print_warn('--storage option is deprecated, use --input to '
+                print(print_warn('--storage option is deprecated, use -i,--input to '
                                  'provide an input bag with a specific storage ID'))
         try:
             storage_options.extend(
@@ -224,7 +229,7 @@ def main(self, *, args):  # noqa: D102
         storage_ids = {options.storage_id for options in storage_options if options.storage_id}
         if storage_config_file and len(storage_ids) > 1:
             print(
-                print_warn('a global --storage-config-file was provided, but --input bags are '
+                print_warn('a global --storage-config-file was provided, but -i,--input bags are '
                            'using different explicit storage IDs, which may lead to errors with '
                            f'replay: {storage_ids}'))