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.

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

Docs: add the "Topics - Command line interface" section #4044

Merged
merged 1 commit into from
May 5, 2020

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented May 2, 2020

Fixes #4015

This content already existed and has been largely moved as is. Just the
style was adapted to match that of the new documentation, such as link
labels, indentation and header styles.

This content already existed and has been largely moved as is. Just the
style was adapted to match that of the new documentation, such as link
labels, indentation and header styles.
@codecov
Copy link

codecov bot commented May 2, 2020

Codecov Report

Merging #4044 into docs-revamp will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           docs-revamp    #4044      +/-   ##
===============================================
- Coverage        78.45%   78.45%   -0.00%     
===============================================
  Files              461      461              
  Lines            34077    34077              
===============================================
- Hits             26732    26731       -1     
- Misses            7345     7346       +1     
Flag Coverage Δ
#django 70.49% <ø> (-<0.01%) ⬇️
#sqlalchemy 71.35% <ø> (ø)
Impacted Files Coverage Δ
aiida/transports/plugins/local.py 80.21% <0.00%> (-0.25%) ⬇️

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 19c8ae4...529bf03. Read the comment docs.

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

LGTM!

Parameters
==========
Parameters to ``verdi`` commands come in two flavors:
Copy link
Contributor

Choose a reason for hiding this comment

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

This particular description always struck me as a bit odd. Maybe we should at least acknowledge that this is UNIX standard?

Suggested change
Parameters to ``verdi`` commands come in two flavors:
Like many other command line utilities, ``verdi`` accepts both regular and optional arguments:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is just a collective term to refer to "arguments" and "options" which are POSIX naming conventions. Of course one can also see options as a special case of command line program arguments. I am not sure if your suggestion would help, because it is not the "optionality" that makes an argument an option. It is the way it is specified. An argument is positional and an option is defined through a flag. An argument can be optional just as well.

Before I merge this: what is it that you find odd about the description? The name parameters? Or the fact that it is not clear that this is a POSIX standard and not just something that AiiDA invented?

Copy link
Contributor

Choose a reason for hiding this comment

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

Before I merge this: what is it that you find odd about the description? The name parameters? Or the fact that it is not clear that this is a POSIX standard and not just something that AiiDA invented?

The fact that it's not made clear that this is POSIX standard and not something that AiiDA invented. I also find the word flavors very odd in this context.

Since we already being pedantic about naming, as a far as I know, I don't think we are using the POSIX terms correctly.

  1. The word "parameter" refers to actual values provided as argument.
  2. The "positional arguments" should actually be referred to as "operands".
  3. A "flag" is an historical term for an option argument, it does not necessarily refer to the convention of prefixing an argument with one or two hyphens.

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
https://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap12.html

I'm happy to defer that discussion to a separate issue / PR and just merge this as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I love a good bit of pedantry :) point well taken. I have to admit that the terminology has also been adopted from click. Fine to merge this now and discuss this at a later point.

@sphuber sphuber added the pr/on-hold PR should not be merged label May 4, 2020
@sphuber
Copy link
Contributor Author

sphuber commented May 4, 2020

Thanks for the review @csadorf I am putting this on hold because I think it is better to first merge the last remaining scaffolding PR. Also I would like to try to improve the description of parameters if you think it is odd.

@sphuber sphuber merged commit bb3995c into aiidateam:docs-revamp May 5, 2020
@sphuber sphuber deleted the fix/4015/docs-topics-cli branch May 5, 2020 11:48
csadorf pushed a commit that referenced this pull request May 29, 2020
This content already existed and has been largely moved as is. Just the
style was adapted to match that of the new documentation, such as link
labels, indentation and header styles.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/on-hold PR should not be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants