Skip to content

Update quickstart documentation #69

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 1, 2020
Merged

Update quickstart documentation #69

merged 1 commit into from
Dec 1, 2020

Conversation

nmaynes
Copy link
Contributor

@nmaynes nmaynes commented Nov 28, 2020

Title cased section headings, added more description to section on get_id_of_title, changed some language to be clearer.

@nmaynes
Copy link
Contributor Author

nmaynes commented Nov 28, 2020

This is my first sphinx documentation change so please let me know if there are specific options to use when building the docs.

@nmaynes
Copy link
Contributor Author

nmaynes commented Nov 28, 2020

Something does not look right. If someone can help me with the sphinx command I will rerun.

I ran python setup.py build_sphinx --source-dir=docs/ --build-dir=docs/build --all-files as instructed in the README but I do not think that is correct.

@shcheklein
Copy link
Member

python setup.py build_sphinx --source-dir=docs/ --build-dir=docs/build --all-files - that should be correct. Do you experience any problems with that? You might need to install some packages:

pip install sphinx
pip install sphinx_rtd_theme

shcheklein
shcheklein reviewed Nov 28, 2020
View reviewed changes
docs/build/html/_sources/quickstart.rst.txt Outdated
``get_id_of_title`` is a function that return id of the given title's file in the directory.
A common task is providing the Google Drive API with a file id.
``get_id_of_title`` is a function that returns the id of a file handle by searching the file titles in a
given directory. The function takes two arguments, title and parent_directory_id. Title is a string that
Copy link
Member

Choose a reason for hiding this comment

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

I know that it's not a rule yet across these docs (they haven't been taken care of for a while at Google), but let's do inline code for var names - parent_directory_id, title, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the formatting. Will try and get the tests to run locally to see where this PR is having problems.

Copy link
Member

Choose a reason for hiding this comment

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

ah, no need I think, CI fails because it can't run tests from a fork (tests depend on credentials that are needed to access GDrive test instance). I don't any reasons for this PR to break tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am also concerned with the missing lines of code. It appears this PR removes ~1,000 lines and I am not sure what they are for. If you want to review the updates and it looks good, go ahead and merge it. I may submit some changes in the future as I have started using this library for a project and want the docs to be clear for new team members and anyone else who starts using PyDrive.

Copy link
Member

Choose a reason for hiding this comment

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

feel free to remove any changes to the build directory (just test it locally, that it builds w/o any warnings and you can open it). I can update the build later. (Actually, we need automate this process I think and this using GitHub actions + put build files to a different branch- feel free to create a ticket for this.)

I may submit some changes in the future as I have started using this library for a project and want the docs to be clear for new team members and anyone else who starts using PyDrive.

that would be great! thank you!

shcheklein
shcheklein reviewed Nov 28, 2020
View reviewed changes
nmaynes
nmaynes commented Nov 28, 2020
View reviewed changes
Copy link
Contributor Author

@nmaynes nmaynes left a comment

Choose a reason for hiding this comment

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

It looks fine to me other than the missing portions of the BIN files and the genindex.html file.

nmaynes
nmaynes commented Nov 28, 2020
View reviewed changes
Copy link
Contributor Author

@nmaynes nmaynes left a comment

Choose a reason for hiding this comment

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

Small changes to BIN files in build directory.

@shcheklein
Copy link
Member

@nmaynes I still see some changes to the docs/build directory? Do you need a hand with this? (to remove those from the PR)?

After this PR is merged I can run the docs build and update the website.

nmaynes
nmaynes commented Dec 1, 2020
View reviewed changes
Copy link
Contributor Author

@nmaynes nmaynes left a comment

Choose a reason for hiding this comment

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

I have gone ahead and removed all the changes from the build directory and pushed only the quickstart.rst changes. I think this will allow for regenerating the Website with no problems.

@shcheklein shcheklein merged commit afbec92 into iterative:master Dec 1, 2020
@shcheklein
Copy link
Member

@nmaynes thanks ! I'll update the website soon.

shcheklein added a commit that referenced this pull request Dec 1, 2020
@shcheklein
rebuild docs after #69 got merged
ee36831
@shcheklein
Copy link
Member

@nmaynes released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shcheklein shcheklein shcheklein approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nmaynes @shcheklein