-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Break up arguments section #2586
Conversation
docs/arguments.rst
Outdated
Like options, arguments can also grab values from an environment variable. | ||
Unlike options, however, this is only supported for explicitly named | ||
environment variables. | ||
This feature is not recommended since it be confusing to users. Arguments can only pull environment variables from ? explicitly named environment variables. In that case, it can also be a list of different environment variables where the first one is picked. ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndreasBackx Do you know what the documentation could mean in this section? I have not a clue so providing clearer wording and a better example is difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure myself, I'd have to look in the code. I'd have a look around the implementation or look at the blame of that documentation, maybe the commit/PR introducing it has some history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndreasBackx I think actually only Armin knew. It was basically unchanged since the first commit. But I figured it out by playing around. Could you give this PR a once over?
517df5a
to
424d0e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I really like about these changes is that some of this new documentation is so much more easily skimmable. Instead of rather large paragaphs, we have easy to understand lists. I am a big fan of this and we should to this elsewhere in the documentation as well!
Most of my comments are improvements towards:
- Consistency in writing.
- Adding typing which aids with what we're documenting here.
- Some regressions I think we're making somewhere.
docs/handling-files.rst
Outdated
File open behavior can be controlled by the boolean kwarg ``lazy``. If a file is opened lazily: | ||
|
||
* A failure at first IO operation will happen by raising an :exc:`FileError`. | ||
* It can help minimize resource handling confusion. If a file is opened in lazy mode, it will receive a ``close_intelligently`` method that can help figure out if the file needs closing or not. This is not needed for parameters, but is necessary for manually prompting. For manual prompts with the :func:`prompt` function you do not know if a stream like stdout was opened (which was already open before) or a real file was opened (that needs closing). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a page anywhere where close_intelligently
is documented? Would be nice to link to it because this does not explain that enough imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it does not explain enough. It seems like it refers to specific parts of the code but I am not sure which.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It refers to LazyFile.close_intelligently
. Could you add that reference? That should be good for now. LazyFile
is in utils.
429d5d4
to
e4575dd
Compare
e4575dd
to
9959aa9
Compare
Was not able to include full type hints because of issues with custom sphinx directive we use to run the code. |
Break up the arguments section into arguments and handling files.