-
Notifications
You must be signed in to change notification settings - Fork 394
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
gdrive: update docs #926
gdrive: update docs #926
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make a separate section in the User Guide and put a link to that page here. Too much content for the command reference expandable section, the link to the bottom of the page is confusing (can be probably improved, but it's better just to solve this properly). The flow is not natural - in the create project you already use dvc remote add root/path
and explanation to this comes later.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! just a few minor comments to address.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good iteration! @jorgeorpinel could you try it and give us feedback if it's clear and you was able to setup it? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Max. Definitely a great idea to extract this into a separate guide. Here's a first round of my review.
Some of my comments are small language things that I should probably help addressing myself, but I'm spread a little thin today so take a look if you can. Some of them are more general and can apply even to text I didn't specifically mention so they definitely deserve some consideration.
This comment has been minimized.
This comment has been minimized.
I ran into a bug (iterative/dvc/issues/3235) so I can't actually try the URLs unfortunately. But maybe we should ask other people to try all this anyway @shcheklein? I'll test them once the bug is resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Check some comments around. Did you manage to get access after all?
@jorgeorpinel anything else left here? |
@Maxris anything else left, can you resolve/respond to the comments? |
@shcheklein done, have rebased 17 previous commits into single 1 as well. |
merging this, @Maxris please create a separate PR with those adjustments you had in mind please. |
@shcheklein going to resolve merging conflict |
@shcheklein done |
Sorry totally missed track of this one but I see you guys resolved everything. I like with the final result, thanks! |
Fixes #917
Disregard the recommendations below if you use Edit on GitHub button to improve the docs 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 enables GitHub to link the PR to the corresponding bug and close it automatically when PR is merged.Thank you for the contribution - we'll try to review and merge it as soon as possible. 🙏