Skip to content

Conversation

@pmrowla
Copy link
Contributor

@pmrowla pmrowla commented Feb 18, 2020

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please chose to allow us to edit your branch when creating the PR.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

Documentation changes for treeverse/dvc#3343

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.

Hi @pmrowla. Thanks for this! Looks great so far but please ping us again (you can request my review for example) once treeverse/dvc/pull/3343 is approved.

The CI checks are failing as of now though. yarn format-check specifically. Please run yarn so you have the pre-commit hook that checks/fixes this before committing. See https://dvc.org/doc/user-guide/contributing/docs#development-environment for more info.

@pmrowla
Copy link
Contributor Author

pmrowla commented Feb 19, 2020

I had run yarn, but it looks like the pre-commit hook wasn't installed for some reason? On my local machine I do have git configured to install some pre-commit hooks by default for every repo that I clone, so maybe that's why.

It was not an issue when I cloned the dvc source repo, the source pre-commit hook was installed properly there.

@shcheklein
Copy link
Contributor

@pmrowla an alternative is to run yarn format-all (or something like this - check the package.json please).

@pmrowla
Copy link
Contributor Author

pmrowla commented Feb 19, 2020

Yeah, I found where the pre-commit hook is specified in package.json and added it to my .git/hooks manually.

@shcheklein
Copy link
Contributor

@pmrowla sounds good! it interesting though why husky didn't install it after running yarn 🤔 not a big deal though

@pmrowla
Copy link
Contributor Author

pmrowla commented Feb 19, 2020

One thing to note, I took the original docs for password/ask_password from the SSH remote docs, so the usage/clarification issues noted by @shcheklein could also probably be cleaned up there as well.

Copy link
Contributor

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

is it still WIP?

@shcheklein
Copy link
Contributor

@pmrowla please add http://user@example.com/path to link checker exclusion list (see CircleCi complaints)

@jorgeorpinel could you please take a brief look, suggest some final edits here?

@pmrowla pmrowla changed the title [WIP] remote: Document HTTP remote push changes remote: Document HTTP remote push changes Feb 20, 2020
- add http://user@example.com/path
@jorgeorpinel
Copy link
Contributor

Of course, but I think we need to wait for treeverse/dvc/pull/3343 to merge before finalizing this.

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.

A bunch of language suggestions. Some more relevant than others. Thanks

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.

I actually approve this. But please let us know when treeverse/dvc/pull/3343 is merged, to check whether there are any further changes there that need to be reflected here. Thanks @pmrowla!

@pmrowla pmrowla requested a review from jorgeorpinel February 21, 2020 10:14
@pmrowla
Copy link
Contributor Author

pmrowla commented Feb 21, 2020

treeverse/dvc/pull/3343 has been merged, there were no changes in the source PR since the docs changes were last approved

@jorgeorpinel
Copy link
Contributor

Thank you so much for keeping docs up to date!

@pmrowla pmrowla deleted the http-push branch November 29, 2022 10:27
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.

3 participants