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

cmd: rephrase intro of run #1537

Merged
merged 1 commit into from
Jul 6, 2020
Merged

cmd: rephrase intro of run #1537

merged 1 commit into from
Jul 6, 2020

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Jul 6, 2020

No description provided.

@shcheklein shcheklein temporarily deployed to dvc-landing-cmd-run-int-z24hjr July 6, 2020 20:08 Inactive
@jorgeorpinel jorgeorpinel changed the title cmd: rephrase intro of run cmd: rephrase intro of run, et al. Jul 6, 2020
@jorgeorpinel jorgeorpinel changed the title cmd: rephrase intro of run, et al. cmd: rephrase intro of run Jul 6, 2020
@jorgeorpinel jorgeorpinel requested a review from shcheklein July 6, 2020 20:11
Copy link
Contributor Author

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

@efiop please notice these small changes, WDYT?

usage: dvc run [-h] [-q | -v] [-d <path>] [-n <name>] [-o <path>]
usage: dvc run [-h] [-q | -v] -n <name> [-d <path>] [-o <path>]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If -n is required, why is is printed in [] here?

Copy link
Contributor

@efiop efiop Jul 7, 2020

Choose a reason for hiding this comment

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

@jorgeorpinel Because it is not strictly required, we have a hidden --single-stage hack for which it is not required. Thanks for fixing it in the docs!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok good to know.

P/s should the -h output

  -n NAME, --name NAME  Stage name.

Say it's (usually) required?

command Command to execute.
command Command for the stage. Executed by default
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Help desc. update!

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Jul 6, 2020

Choose a reason for hiding this comment

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

I can submit a PR on the core repo if you agree, Ruslan. Cc @pared @skshetry

Copy link
Member

Choose a reason for hiding this comment

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

Looks really strange w/o a dot at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I think this is actually a full sentence (with implicit subject & verb "It's").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing in #1540

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Jul 8, 2020

Choose a reason for hiding this comment

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

technically, the command may or may not execute (think of run-cache

If it's in the run cache it means the results are applied at dvc run again. From the user's perspective this is the same as executing it again (instantly).

you want to do the PR on core

Yes, will do... ⏳

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will do... ⏳

Actually, since there's some controversy here, I'll let you guys decide @pared and @skshetry and if you do not change it just lmk in order to roll this one back.

Copy link
Member

Choose a reason for hiding this comment

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

@jorgeorpinel, I don't see any difference between previous and this version. It only adds confusion (what does by default mean?). It'd be better to explain the cases where it won't be executed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @skshetry is right, I don't think we need to explain when it's not executed, thats is what --no-exec reference is for. Maybe we should replace Only create stage file without actually running it. -> Create stage file without actually executing its command. Since we are operating on command and execute in main ref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skshetry you don't see a difference between "Command to execute" and "Command for the stage" ? The former reflected run being a command to run commands (what it used to be), the latter reflects it being a helper to define stages (what it is now, see the cmd ref Description).

I thought we were just discussing the "Executed by default" which I don't have a strong opinion on, tbh. Removed in #1550 (let's continue this part of the discussion there?)

Maybe we should replace Only create stage file without actually running it. -> Create stage file without actually executing its command.

@pared I agree. Or use the text from the cmd ref: create a stage file, but do not execute the command defined in it, nor cache dependencies or outputs

@shcheklein shcheklein merged commit 0f63c2b into master Jul 6, 2020
jorgeorpinel added a commit that referenced this pull request Jul 6, 2020
shcheklein pushed a commit that referenced this pull request Jul 7, 2020
* cmd: rephrase intro of run
per #1526 (comment)

* cmd: typo in run -h out
per #1537 (comment)
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.

5 participants