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

Adding more text for exclusive vs inclusive parameters and added diagram #384

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

acoleman2000
Copy link

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.

Copy link
Member

@kinow kinow left a 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.

image

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.
Copy link
Member

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.
Copy link
Member

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
```


Copy link
Member

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.

@zoe-translates
Copy link
Contributor

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 ag -iG '^.+\.md$' 'inclusive' in the src directory (i.e. print all occurrence of the word "inclusive" in markdown files), the only hit is this one here.

I think what this means is that we have reason to do away with the word "inclusive" here and replace it with "dependent".

@zoe-translates
Copy link
Contributor

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)

This should be fixed by f940a62. A rebase should fix the build error.

@mr-c mr-c force-pushed the exclusive_inclusive_parameters branch from c047876 to 9a0d0b6 Compare June 29, 2023 02:40
@mr-c mr-c force-pushed the exclusive_inclusive_parameters branch from 9a0d0b6 to 25721d1 Compare June 29, 2023 04:55
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants