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

Add docs about pager in the pipeline command #831

Merged
merged 18 commits into from Dec 11, 2019
Merged

Add docs about pager in the pipeline command #831

merged 18 commits into from Dec 11, 2019

Conversation

ghost
Copy link

@ghost ghost commented Dec 2, 2019

Fixes #836

@shcheklein

This comment has been minimized.

@ghost

This comment has been minimized.

@ghost ghost marked this pull request as ready for review December 8, 2019 13:28
@ghost ghost changed the title Add docs about pager in the pipeline command WIP Add docs about pager in the pipeline command Dec 8, 2019
@ghost ghost changed the title WIP Add docs about pager in the pipeline command Add docs about pager in the pipeline command Dec 8, 2019
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Good stuff!

There are other places that explain navigation by W, A, etc - please do git grep

Also, check some comments I left.

Tymoteusz Jankowski added 3 commits December 9, 2019 20:57
1. add dvc or bash to code blocks
2. remove a reference of WASD
3. replace a code comment with a paragraph
@shcheklein shcheklein temporarily deployed to dvc-landing-2807-use-pa-emgsrg December 9, 2019 20:26 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-2807-use-pa-emgsrg December 9, 2019 21:12 Inactive
@ghost

This comment has been minimized.

@ghost
Copy link
Author

ghost commented Dec 9, 2019

@xliiv build is failing, pre-commit is not installed? check the docs contributing guide, please to install it

I installed it yesterday.. i'm not sure why the invalid commit passed.. .
Maybe it's related to vim's fugitive where i do commits?

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

👍

static/docs/command-reference/pipeline/show.md Outdated Show resolved Hide resolved
static/docs/command-reference/pipeline/show.md Outdated Show resolved Hide resolved
static/docs/command-reference/pipeline/show.md Outdated Show resolved Hide resolved
static/docs/command-reference/pipeline/show.md Outdated Show resolved Hide resolved
static/docs/command-reference/pipeline/show.md Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Some specific notes below. Please also spell check all your changes (I fixed a few typos).

static/docs/command-reference/pipeline/show.md Outdated Show resolved Hide resolved
static/docs/user-guide/running-dvc-on-windows.md Outdated Show resolved Hide resolved
static/docs/user-guide/running-dvc-on-windows.md Outdated Show resolved Hide resolved
static/docs/user-guide/running-dvc-on-windows.md Outdated Show resolved Hide resolved
@shcheklein shcheklein temporarily deployed to dvc-landing-2807-use-pa-emgsrg December 11, 2019 19:43 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-2807-use-pa-emgsrg December 11, 2019 19:55 Inactive
@ghost
Copy link
Author

ghost commented Dec 11, 2019

Some specific notes below. Please also spell check all your changes (I fixed a few typos).

good catch, checked and fixed two words.. 👍

Copy link
Contributor

@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.

Almost there. Thanks!

@shcheklein shcheklein temporarily deployed to dvc-landing-2807-use-pa-emgsrg December 11, 2019 20:29 Inactive
static/docs/command-reference/pipeline/show.md Outdated Show resolved Hide resolved
static/docs/command-reference/pipeline/show.md Outdated Show resolved Hide resolved
static/docs/user-guide/running-dvc-on-windows.md Outdated Show resolved Hide resolved
static/docs/user-guide/running-dvc-on-windows.md Outdated Show resolved Hide resolved
@shcheklein shcheklein temporarily deployed to dvc-landing-2807-use-pa-emgsrg December 11, 2019 20:39 Inactive
Copy link
Contributor

@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.

Last details I think

static/docs/user-guide/running-dvc-on-windows.md Outdated Show resolved Hide resolved
@shcheklein shcheklein temporarily deployed to dvc-landing-2807-use-pa-emgsrg December 11, 2019 20:58 Inactive
Copy link
Contributor

@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.

Shoot sorry, one last detail ^

```

`less` can be installed in other ways, just make sure it's available in
`cmd`/Powershell, where you run dvc. (This usually means adding the directory
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`cmd`/Powershell, where you run dvc. (This usually means adding the directory
`cmd`/Powershell, where you run `dvc`. (This usually means adding the directory

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I can commit this 🙂 One min...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah no, sorry. I don't have permission to push to your fork. Please image

Copy link
Contributor

Choose a reason for hiding this comment

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

(p.s. FYI you can allow upstream repo maintainers to push to the branch in the PR settings.)

Copy link
Member

Choose a reason for hiding this comment

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

@jorgeorpinel let's merge and fix it in-place? :) it's too minor to do a cycle.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, I can definitely do that!

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in 9955833.

@shcheklein shcheklein merged commit 917be5a into iterative:master Dec 11, 2019
@jorgeorpinel
Copy link
Contributor

Thanks for the contribution and addressing all the feedback so promptly @xliiv !

jorgeorpinel added a commit that referenced this pull request Dec 11, 2019
@ghost
Copy link
Author

ghost commented Dec 12, 2019

Thanks for the contribution and addressing all the feedback so promptly @xliiv !

thank you for your review!

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.

add info about pager on Windows
3 participants