Arvados

Some comments:

• sdk/python/arvados/commands/arv_copy.py file:
  • Line 116: Command’s description could be updated to reflect the new feature
  • Lines 412-422: Should that documentation be as a docstring inside the func?
  • Line 438: Should wf_def be passed instead of graph?
  • Lines 458-460: To improve readability, do you think it would be better to replace those 3 lines with a single assignment like: var = one() or another() ?
  • Line 468: I believe that if can be replaced with elif
• As you mentioned, I too think that ruamel.yaml should be added as a dependency. It seems that Python doesn’t have ALL the batteries included after all.
• Do you think a test for this feature would be desirable?

Line 116: Command’s description could be updated to reflect the new feature

You are right. Updated.

Lines 412-422: Should that documentation be as a docstring inside the func?

The documentation for recursive, project_uuid etc already exists. This comment is additional code comment only, aimed at code maintenance.

Line 438: Should wf_def be passed instead of graph?

Thanks for catching this. Corrected.

Lines 458-460: To improve readability, do you think it would be better to replace those 3 lines with a single assignment like: var = one() or another() ?

Updated and thanks for teaching :)

Line 468: I believe that if can be replaced with elif

Corrected

As you mentioned, I too think that ruamel.yaml should be added as a dependency. It seems that Python doesn’t have ALL the batteries included after all.

Added ruamel.yaml to setup.py per Peter's suggestion

Do you think a test for this feature would be desirable?

Unfortunately, this feature has no automated tests yet and Peter agreed that I only manually test for now.

Thanks.

LGTM. Thanks!