From 06f8f4cfb0731ff699d5c01ad85418b6db0f6778 Mon Sep 17 00:00:00 2001 From: Sebastiaan Huber Date: Tue, 28 May 2024 22:54:00 +0200 Subject: [PATCH] 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. --- src/aiida/cmdline/commands/cmd_node.py | 44 ++++---------------------- tests/cmdline/commands/test_node.py | 16 +++------- 2 files changed, 11 insertions(+), 49 deletions(-) diff --git a/src/aiida/cmdline/commands/cmd_node.py b/src/aiida/cmdline/commands/cmd_node.py index 2171542d2e..8e6ae8fba0 100644 --- a/src/aiida/cmdline/commands/cmd_node.py +++ b/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}`') diff --git a/tests/cmdline/commands/test_node.py b/tests/cmdline/commands/test_node.py index b121173b8c..d9c7774556 100644 --- a/tests/cmdline/commands/test_node.py +++ b/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)