CLI: verdi node graph generate root nodes as arguments (#6427)

Recently, the command was changed to support specifying multiple root
nodes. To keep backwards compatibility, the `-N/--nodes` option was
added. This led to some pretty awkward behavior though with also the
output file being defined as an argument being deprecated in favor of
the `-O/--output-file` option.

If backwards compatibility wasn't a concern, a better interface would be
to take root nodes as positional arguments, which is the standard across
`verdi` commands. Since this is a more intuitive and consistent design,
it is adopted here despite it breaking backwards compatibility.

Author: sphuber

src/aiida/cmdline/commands/cmd_node.py

@@ -425,8 +425,7 @@ def verdi_graph():
 
 
 @verdi_graph.command('generate')
-@arguments.NODE('root_node', required=False)
-@options.NODES(help='The root node(s) whose provenance graph to include.')
+@arguments.NODES('root_nodes')
 @click.option(
     '-l',
     '--link-types',
@@ -477,15 +476,12 @@ def verdi_graph():
 @click.option(
     '-O',
     '--output-file',
-    'output_filename',
     type=click.Path(dir_okay=False, path_type=pathlib.Path),
     help='The file to write the output to.',
 )
-@arguments.OUTPUT_FILE(required=False)
 @decorators.with_dbenv()
 def graph_generate(
-    root_node,
-    nodes,
+    root_nodes,
     link_types,
     identifier,
     ancestor_depth,
@@ -496,44 +492,18 @@ def graph_generate(
     output_format,
     highlight_classes,
     show,
-    output_filename,
     output_file,
 ):
     """Generate a graph from one or multiple root nodes."""
     from aiida.cmdline.utils import echo
     from aiida.tools.visualization import Graph
 
-    if root_node and nodes:
-        echo.echo_warning(
-            'Specifying the root node positionally and the `-N/--nodes` option at the same time is not supported, '
-            'ignoring the `ROOT_NODE`. Please use the `-N/--nodes` option only.'
-        )
-        root_node = None
-
-    if root_node:
-        echo.echo_deprecated(
-            'Specifying the root node positionally is deprecated, please use the `-N/--nodes` option instead.'
-        )
-
-    root_nodes = nodes or [root_node]
-
-    if output_file and output_filename:
-        echo.echo_warning(
-            'Specifying the output file positionally and the `-O/--output-file` option at the same time is not '
-            'supported, ignoring the `OUTPUF_FILE`. Please use the `-O/--output-file` option only.'
-        )
-        output_file = None
-
-    if output_file:
-        echo.echo_deprecated(
-            'Specifying the output file positionally is deprecated, please use the `-O/--output-file` option instead.'
-        )
-
-    output_filename = output_file or output_filename
+    if not root_nodes:
+        echo.echo_critical('No root node(s) specified.')
 
-    if not output_filename:
+    if not output_file:
         pks = '.'.join(str(n.pk) for n in root_nodes)
-        output_filename = pathlib.Path(f'{pks}.{engine}.{output_format}')
+        output_file = pathlib.Path(f'{pks}.{engine}.{output_format}')
 
     echo.echo_info(f'Initiating graphviz engine: {engine}')
     graph = Graph(engine=engine, node_id_type=identifier)
@@ -559,7 +529,7 @@ def graph_generate(
             highlight_classes=highlight_classes,
         )
 
-    filename_written = graph.graphviz.render(outfile=output_filename, format=output_format, view=show, cleanup=True)
+    filename_written = graph.graphviz.render(outfile=output_file, format=output_format, view=show, cleanup=True)
 
     echo.echo_success(f'Output written to `{filename_written}`')
 

tests/cmdline/commands/test_node.py

@@ -278,21 +278,13 @@ def test_generate_graph(self, run_cli_command):
             delete_temporary_file(filename)
 
     def test_multiple_nodes(self, run_cli_command):
-        """Test the `-N/--nodes` option to specify multiple root nodes."""
+        """Test multiple root nodes."""
         node = orm.Data().store()
-        options = ['-N', str(self.node.pk), str(node.pk)]
+        options = [str(self.node.pk), str(node.pk)]
         result = run_cli_command(cmd_node.graph_generate, options)
         assert 'Success: Output written to' in result.output
         assert os.path.isfile(f'{self.node.pk}.{node.pk}.dot.pdf')
 
-    def test_deprecation_warnings(self, run_cli_command):
-        """Test that the positional arguments are deprecated and emit a warning."""
-        options = [str(self.node.pk), 'output.pdf']
-        result = run_cli_command(cmd_node.graph_generate, options)
-        assert 'Specifying the root node positionally is deprecated' in result.output
-        assert 'Specifying the output file positionally is deprecated' in result.output
-        assert os.path.isfile('output.pdf')
-
     def test_catch_bad_pk(self, run_cli_command):
         """Test that an invalid root_node pk (non-numeric, negative, or decimal),
         or non-existent pk will produce an error
@@ -409,11 +401,11 @@ def test_node_id_label_format(self, run_cli_command):
 
     @pytest.mark.parametrize('output_file', ('without_extension', 'without_extension.pdf'))
     def test_output_file(self, run_cli_command, output_file):
-        """Test that the output file can be specified through an argument."""
+        """Test that the output file can be specified through ``-O/--output-file``."""
         with warnings.catch_warnings():
             warnings.simplefilter('ignore')
             try:
-                run_cli_command(cmd_node.graph_generate, [str(self.node.pk), output_file])
+                run_cli_command(cmd_node.graph_generate, [str(self.node.pk), '--output-file', output_file])
                 assert os.path.isfile(output_file)
             finally:
                 delete_temporary_file(output_file)