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

docs: API auto-mentioning in docs (#910) #1305

Merged
merged 8 commits into from
May 19, 2020

Conversation

mvshmakov
Copy link
Contributor

Fix #910

@mvshmakov
Copy link
Contributor Author

There is one thing: I don't know whether I should consider the proposed by CodeClimate "Cognitive Complexity" issue or not due to the fact that the commandLinker has the same complexity and it was in the production already.

@@ -139,7 +139,7 @@ current working directory (anywhere in the file system with user write access).

```dvc
$ dvc import https://github.com/example/registry \
images/faces/
Copy link
Member

Choose a reason for hiding this comment

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

same here - not sure what's going on

@shcheklein shcheklein requested a review from rogermparent May 18, 2020 22:20
@shcheklein
Copy link
Member

There is one thing: I don't know whether I should consider the proposed by CodeClimate "Cognitive Complexity" issue or not due to the fact that the commandLinker has the same complexity and it was in the production already.

CC is too aggressive and we try to fix those issue, but it's not a requirement - especially complexity and number of lines of code

@shcheklein shcheklein temporarily deployed to dvc-landing-feature-910-23uvld May 18, 2020 22:23 Inactive
@shcheklein
Copy link
Member

Deployed a preview (it does not happen automatically on forks, usually we use branches) - https://dvc-landing-feature-910-23uvld.herokuapp.com/

@shcheklein shcheklein requested a review from jorgeorpinel May 18, 2020 22:34
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.

LGTM 😎 only some small issues with code block formatting. I left a few comments.

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.

Looks good. I just wonder if we need dvc.api to link to the API ref. Probably yes but maybe also remove some manual links that make it repetitive, like this one from https://dvc-landing-feature-910-23uvld.herokuapp.com/doc/install

image

Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

I'm not particularly knowledgeable about the internal DVC linker plugin, but the code itself looks good to me.

@mvshmakov I have to ask: is your editor automatically changing the indentation of the MD code blocks @shcheklein pointed out or is it a deliberate choice? I don't have strong feelings either way, but it doesn't seem to be our linter.

It's probably best to revert just the indentation-only changes to how they were originally, and if there's a legitimate reason to change them we can address that as its own issue. Either way, nice contribution! Thanks!

@shcheklein shcheklein temporarily deployed to dvc-landing-feature-910-23uvld May 19, 2020 10:34 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-feature-910-23uvld May 19, 2020 10:57 Inactive
@mvshmakov
Copy link
Contributor Author

Looks good. I just wonder if we need dvc.api to link to the API ref. Probably yes but maybe also remove some manual links that make it repetitive, like this one from https://dvc-landing-feature-910-23uvld.herokuapp.com/doc/install

image

I have removed what you suggested, and I can't find anything like this anymore.

Our Python API, included with the `dvc` package installed with DVC, includes the
`open` function to load/stream data directly from external <abbr>DVC
projects</abbr>:
Our Python API (`dvc.api`), included with the `dvc` package installed with DVC,
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I would actually prefer to link [Python API] manually for readability.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in 036672d along with a couple more instances I found.

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.

Thanks, LGTM! Just left an optional suggestion ☝️

@shcheklein
Copy link
Member

@jorgeorpinel I will merge this (since you approved and to move forward), let's update that text as part of the regular PRs?

@jorgeorpinel
Copy link
Contributor

Sure!

@shcheklein shcheklein merged commit 24d7385 into iterative:master May 19, 2020
jorgeorpinel added a commit that referenced this pull request May 20, 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.

automatic links for API function names (to api ref)
4 participants