Skip to content

cmd: copy edits for import-url ref.#1494

Merged
jorgeorpinel merged 15 commits into
treeverse:masterfrom
utkarshsingh99:patch12
Jul 7, 2020
Merged

cmd: copy edits for import-url ref.#1494
jorgeorpinel merged 15 commits into
treeverse:masterfrom
utkarshsingh99:patch12

Conversation

@utkarshsingh99
Copy link
Copy Markdown
Contributor

@utkarshsingh99 utkarshsingh99 commented Jun 24, 2020

  • ❗ 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 choose 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. 🙏

Partially address #1469

Comment thread content/docs/command-reference/add.md Outdated
Comment thread content/docs/command-reference/import-url.md Outdated
@jorgeorpinel
Copy link
Copy Markdown
Contributor

You could add the "This" -> "These" update here 😉

@utkarshsingh99
Copy link
Copy Markdown
Contributor Author

Fix #1492

@utkarshsingh99
Copy link
Copy Markdown
Contributor Author

utkarshsingh99 commented Jun 27, 2020

@jorgeorpinel I found import-url.md to be the best place for now to differentiate file hashes since all different remote locations are talked about in detail here including their requirements (such as strong ETag, etc) in DVC.
Since import.md (import command) just focuses on the DVC repos or Git repos, they will only have an md5 field, so I guess it isn't worth talking about different file hashes there.
I have added my comments for a few extra changes that I made. Let me know if they are not needed and I will scrap them out. ✌️

Comment thread content/docs/command-reference/import-url.md Outdated
Comment thread content/docs/command-reference/import-url.md Outdated

You may want to get out of and remove the `example-get-started/` directory after
trying this example (especially if trying out the following one).

Copy link
Copy Markdown
Contributor Author

@utkarshsingh99 utkarshsingh99 Jun 27, 2020

Choose a reason for hiding this comment

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

I believe this statement is irrelevant here since anyone could arrive at this page without going through the User Guide/Getting-Started and it does not hold a direct info about the import-url command.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think you followed the entire examples section first. That would need to be done to determine this. Feel free to do so and submit a different PR if you find this unnecessary, but it's out of scope for this PR. Please roll back here. Thanks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Alright.
But do you think it should be removed?

I don't think you followed the entire examples section first.

Actually, since this is in command reference, I thought it would be good to keep this file independent of the Getting Started module

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think it should be removed. But like I mentioned, I would need to follow the entire examples section first to decide.

would be good to keep this file independent of the Getting Started

It is. Again, I don't think you read the whole examples section 🙂

Please roll back.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh okay. Now I get it. It's just related to the example above.
My apologies 😄

@utkarshsingh99 utkarshsingh99 marked this pull request as ready for review June 27, 2020 10:04
@utkarshsingh99

This comment has been minimized.

@jorgeorpinel
Copy link
Copy Markdown
Contributor

Partially address #1469

Please say specifically which part of that issue is addressed in the description of this PR (you can just copy the entire import/import-url checkbox here, and mark it).

Fix #1492

FYI special keywords only work on the PR description. Not needed here anyway because I already closed that issue as fixed.

I found import-url.md to be the best place for now to differentiate file hashes since all different remote locations are talked about

I see that import only has md5 in its examples, indeed. What about "add (external dep)" and "run -d (external dep)"? (The other checkboxes in ##1469) Did you check them? You don't have to for this PR, I'm just curious.

The `deps` field in the generated `.dvc` file contains a hash value along with
the `path` to track changes in the remote file/directory. The hash type depends
on the type of remote location (protocol) being tracked. For instance,
files/directories tracked from HTTP(s)/S3/Azure/GS remote locations will have an
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I made a mistake with the comment. GS uses md5. Sorry about that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll change it in the DVC Files and Directories page in the next commit ✌️

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel Jul 4, 2020

Choose a reason for hiding this comment

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

No worries @skshetry, glad you caught it though! Thanks for the followup. I updated #1469 and fixed the dvc.lock doc in another PR. But this brings some questions... I'll ping you somewhere else.

@utkarshsingh99 please update the paragraph above accordingly...

Comment thread content/docs/command-reference/import-url.md Outdated
@jorgeorpinel

This comment has been minimized.

@utkarshsingh99
Copy link
Copy Markdown
Contributor Author

What about "add (external dep)" and "run -d (external dep)"? (The other checkboxes in ##1469) Did you check them?

There's one mention of adding external files in dvc add but it's linking already to the External Dependencies page. Perhaps we can add a link to hash values' info here: dvc add example?

Comment thread content/docs/user-guide/dvc-files-and-directories.md Outdated
@jorgeorpinel
Copy link
Copy Markdown
Contributor

Nah actually add is a special case. Those aren't deps, they're outs. Don't worry about that for now.

But what about dvc run -d? I assume you didn't check it and that's fine. Let's just focus on addressing the remaining feedback up here ☝️ please, so we can merge this.

@jorgeorpinel jorgeorpinel changed the title added info about different file hashes cmd: added info about different file hashes to import-url Jul 4, 2020
@utkarshsingh99
Copy link
Copy Markdown
Contributor Author

Removed this

For instance, files/directories tracked from HTTP(s)/S3/Azure remote locations will have an
ETag value in the deps field, while
files/directories tracked from GS/SSH/local locations will have an md5 hash
value. Files from HDFS remote locations use a checksum key in the deps
field.

Added:

See dvc.lock file for more info on hash types for specific remote location types.

Just adding this here since I got a bit confused with 2 reviews here: #1494(comment) and #1494(another comment).
Anyway, let me know if there's something wrong in this change and I'll make the changes immediately. ✌️

Comment thread content/docs/command-reference/import-url.md Outdated
Comment on lines +91 to +96
The `deps` field in the generated `.dvc` file contains a hash value along with
the `path` to track changes in the remote file/directory. The hash type depends
on the type of remote location (protocol) being tracked. See
[`dvc.lock` file](/doc/user-guide/dvc-files-and-directories#dvclock-file) for
info on hash types for specific remote location types.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah this whole paragraph is probably unnecessary but thanks. The ref. is already too long TBH. Users can find out about .dvc file specs in its dedicated guide, that's not the main purpose for a command ref.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, so where should I link dvc.lock file ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why did you decide to edit this command reference if there's no existing places that relate to dvc.lock? I'm going to just merge this with the few typo fixed but I'll have to uncheck that box in the original index so this can be re-evaluated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I actually thought that we needed to add some reference to the hash values in each file or describe where it was needed based on the original issue. But does this mean that there is no place we can add a reference to dvc.lock or provide info about different types of file hashes in import-url?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

does this mean that there is no place we can add a reference to dvc.lock or provide info about different types of file hashes in import-url?

Idk, can you answer that question? 🙂 Let's go back to analyzing the checkboxes of #1469 and please comment there if any one seems unaplicable.

@jorgeorpinel
Copy link
Copy Markdown
Contributor

confused with 2 reviews

Ah yeah, it's the first one: remove whole paragraph. Thanks

Comment thread content/docs/command-reference/import-url.md Outdated
@jorgeorpinel jorgeorpinel changed the title cmd: added info about different file hashes to import-url cmd: copy edits for import-url ref. Jul 7, 2020
@jorgeorpinel jorgeorpinel merged commit 4426244 into treeverse:master Jul 7, 2020
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