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: updated README.rst file #563

Merged
merged 9 commits into from Dec 15, 2020
Merged

Docs: updated README.rst file #563

merged 9 commits into from Dec 15, 2020

Conversation

mf2199
Copy link
Contributor

@mf2199 mf2199 commented Dec 2, 2020

Changelog:

  • Restructuring and rephrasing README.rst file to reflect the latest state of development;
  • Removal of DB API driver-related connection-usage.rst file.

@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/python-spanner-django API. label Dec 2, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 2, 2020
@mf2199 mf2199 requested review from c24t and IlyaFaer December 2, 2020 04:57
Copy link
Contributor

@c24t c24t left a comment

Choose a reason for hiding this comment

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

Changes LGTM, just a few nits. I don't think we should check in any "under construction" pages though.

README.rst Outdated
}
}

- In order to retrieve the Cloud Spanner credentials from a JSON file, ``credentials_uri`` parameter can also be supplied in the ``OPTIONS`` field:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- In order to retrieve the Cloud Spanner credentials from a JSON file, ``credentials_uri`` parameter can also be supplied in the ``OPTIONS`` field:
- In order to retrieve the Cloud Spanner credentials from a JSON file, the ``credentials_uri`` parameter can also be supplied in the ``OPTIONS`` field:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

README.rst Outdated
@@ -27,7 +42,7 @@ To install from PyPI:
pip3 install django-google-spanner


To install from source:
To install from the source:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To install from the source:
To install from source:

"Install from source" actually sounds more natural to me. I was curious, but don't know that this proves anything:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Done!

README.rst Outdated
constraints
<https://github.com/googleapis/python-spanner-django/issues/313>`__.
Spanner does not support ``ON DELETE CASCADE`` when creating foreign-key
constraints, so these are not supported in ``django-google-spanner``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
constraints, so these are not supported in ``django-google-spanner``.
constraints, so this is not supported in ``django-google-spanner``.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Done!

README.rst Outdated

Check constraints aren't supported
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Spanner doesn't support ``CHECK`` constraints so one isn't created for
Spanner does not support ``CHECK`` constraints, so one isn't created for
Copy link
Contributor

Choose a reason for hiding this comment

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

"Doesn't" get expanded by "isn't" doesn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Done!

@@ -189,13 +192,11 @@ Spanner doesn't support it. For example:
Schema migrations
~~~~~~~~~~~~~~~~~

Spanner has some limitations on schema changes which you must respect:
There are some limitations on schema changes to consider:
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this one has already been fixed.

Usage
=====

[this page is under construction]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to avoid checking "under construction" pages in, and just add them once they're written.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

README.rst Outdated
To use this library you'll need a Google Cloud Platform project with the Cloud
Spanner API enabled. See the `Cloud Spanner Python client docs
Using this library requires a Google Cloud Platform project with the Cloud
Spanner API enabled. See the Spanner' Python Client `documentation
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Spanner API enabled. See the Spanner' Python Client `documentation
Spanner API enabled. See the Spanner Python client `documentation

I'm no usage expert, but I think "Spanner" and "Python" are capitalized here as separate terms -- not as part of "Spanner Python Client" -- and "client" can be lowercase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Done.

@mf2199 mf2199 marked this pull request as ready for review December 10, 2020 19:22
@mf2199 mf2199 requested a review from a team as a code owner December 10, 2020 19:22
@c24t c24t merged commit d70cb28 into googleapis:master Dec 15, 2020
@IlyaFaer IlyaFaer deleted the docs branch February 1, 2021 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/python-spanner-django API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants