-
-
Notifications
You must be signed in to change notification settings - Fork 68
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
Adding more text for exclusive vs inclusive parameters and added diagram #384
base: main
Are you sure you want to change the base?
Adding more text for exclusive vs inclusive parameters and added diagram #384
Conversation
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.
The rendered HTML with the image and the new text are looking great! Left some comments about the markdown text.
The RTD build is failing. I tried to have a look but couldn't understand what's going on, so I'll revisit it later.
Traceback (most recent call last):
File "/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/envs/384/lib/python3.9/site-packages/sphinx/config.py", line 350, in eval_config_file
exec(code, namespace)
File "/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/384/src/conf.py", line 257, in <module>
from myst_parser.mdit_to_docutils.sphinx_ import SphinxRenderer, create_warning
ImportError: cannot import name 'create_warning' from 'myst_parser.mdit_to_docutils.sphinx_' (/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/envs/384/lib/python3.9/site-packages/myst_parser/mdit_to_docutils/sphinx_.py)
Re-ran the build on RTD, but it failed the same way.
Thanks!
Bruno
``` | ||
|
||
|
||
With cwltool, if not all inclusive arguments are provided, an error is thrown. |
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.
In other pages & paragraphs cwltool appears formatted as cwltool
(the command). We should probably use that here too for consistency (although I noticed there are some places where it's written without the formatting).
@@ -212,7 +222,7 @@ parameters together to describe these two conditions. | |||
:emphasize-lines: 6-7 | |||
``` | |||
|
|||
In the first example, you can't provide `itemA` without also providing `itemB`. | |||
In the first example, you can't provide `itemA` without also providing `itemB`, or an error is thrown. This is because they are inclusive (dependent) parameters. If you provide one, you must provide the other. |
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.
Probably better to break this line to make it shorter like the other ones (regardless of 80
or 120
limits).
This is useful to visualize diffs with git or GitHub UI: https://sembr.org/
:align: center | ||
``` | ||
|
||
|
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 think the extra blank lines are not necessary. This is not really important, but we tried to remove unnecessary blank lines before, so probably better if we can avoid adding extra lines that are not necessary.
Thank you @acoleman2000 for this! One more thing I noticed while doing the translation work in this section. Here we have the word "inclusive" in the section subtitle. That's the only place this word appears in this section (without this PR). It appears to me that the word was being used interchangeably with "dependent". The term "dependent is used in the FAQ, where "inclusive" doesn't appear there. In fact, running I think what this means is that we have reason to do away with the word "inclusive" here and replace it with "dependent". |
This should be fixed by f940a62. A rebase should fix the build error. |
c047876
to
9a0d0b6
Compare
9a0d0b6
to
25721d1
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.
Thanks! Is there a SVG or other vector source for this image?
This pull request is based on this issue.
Add clarifying information about exclusive vs inclusive parameters, as well as including a diagram to illustrate the difference.
Upfront described how cwltool handles exclusive vs inclusive parameters, so user can follow along better with examples.