Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

File name can now be specified for verdi node graph generate #5897

Merged
merged 7 commits into from
Feb 22, 2023
Merged

File name can now be specified for verdi node graph generate #5897

merged 7 commits into from
Feb 22, 2023

Conversation

AhmedBasem20
Copy link
Contributor

Closes #5692
Added a --basename option so that the user could specify a file base name.
if not specified, the default file base name would be the node PK.

sphuber
sphuber requested changes Feb 19, 2023
View reviewed changes
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your contribution @AhmedBasem20 . The solution is just fine, but in this case I think it might be best to let the user fully define the output filename (not just the base name) and also use an argument instead of an option

aiida/cmdline/commands/cmd_node.py Outdated Show resolved Hide resolved
aiida/cmdline/commands/cmd_node.py Outdated Show resolved Hide resolved
tests/cmdline/commands/test_node.py Outdated Show resolved Hide resolved
@AhmedBasem20
Copy link
Contributor Author

Hello @sphuber, Thanks for this informative review. I do agree with you that this solution is more convenient.

@AhmedBasem20 AhmedBasem20 requested a review from sphuber February 20, 2023 23:49
AhmedBasem20 and others added 3 commits February 21, 2023 16:03
@AhmedBasem20
Add output-filename argument for verdi node graph (aiidateam#5692)
1c88000
@AhmedBasem20
Add output-filename argument for verdi node graph (aiidateam#5692)
8f191e3
@sphuber
Fix the output filename handling of verdi node graph generate
a795a66 
The `graphviz.Digraph.render` method would automatically add a suffix to
the output filename when specified with `filename`, which is not what we
want. Using `outfile` instead solves the problem as it doesn't modify the
filename. However, a warning will be issued if the suffix of the
filename does not match the chosen output format, but that is
acceptable.
@sphuber
Copy link
Contributor

sphuber commented Feb 22, 2023

@AhmedBasem20 the tests were failing because the graphviz.Digraph.render method that we were calling was automatically adding another .pdf suffix. This is the case when using the filename argument. I took the liberty to fix it by switching to the outfile argument as that will respect whatever value we specify. Now the tests should work. See the graphviz docs for reference: https://graphviz.readthedocs.io/en/stable/api.html#graphviz.render

@sphuber sphuber merged commit 80045ae into aiidateam:main Feb 22, 2023
@sphuber
Copy link
Contributor

sphuber commented Feb 22, 2023

Thanks for the contribution @AhmedBasem20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sphuber sphuber Awaiting requested review from sphuber

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow to change the filename for verdi node graph generate
2 participants
@AhmedBasem20 @sphuber