-
Notifications
You must be signed in to change notification settings - Fork 31
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
Refactor certain doctests towards either reStructuredText documentation or Python tests #464
Conversation
4a9855f
to
f50be89
Compare
4ad6b09
to
13ed96f
Compare
8b30897
to
a65127e
Compare
135caf7
to
eba6f89
Compare
98630d0
to
48d8bd7
Compare
2695409
to
ac2f4ea
Compare
48d8bd7
to
77dc5df
Compare
ac2f4ea
to
cee3f35
Compare
77dc5df
to
8d8a6d1
Compare
cee3f35
to
13cb63b
Compare
The referenced doctests are now becoming first citizens of the published documentation, within a `by-example` section. To make sure they are always valid, they are still evaluated as doctests. `doctests/mocking.txt` has been translated into a pure-Python test case.
- doctests/sqlalchemy.rst - sqlalchemy/doctests/{dialect,itests,reflection}.txt
This is a preparation for: SQLALCHEMY_WARN_20=1 ./bin/test -vvv -t SqlAlchemy
f36f60f
to
f016016
Compare
f294e3b
to
ddc5e99
Compare
@@ -0,0 +1,122 @@ | |||
===================================================== | |||
SQLAlchemy: Database schema inspection and reflection |
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.
Is anything of this section CrateDB specific? Looks like it's mostly explaining SQLAlchemy, in which case we could refer to the upstream docs instead and test the functionality in python tests
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.
Hi. While the SQLAlchemy API methods per se are not specific to CrateDB, I found this section worth to keep, in order to display different kinds of inspection and reflection methods of SQLAlchemy together with their slightly CrateDB-specific outcomes, also because they are not mentioned elsewhere in our documentation (modulo has_table
), and also because the corresponding spots are a bit obfuscated within the SQLAlchemy documentation.
In other words, while bringing those snippets together from different parts of our documentation, I have made an extra effort to discriminate between the different kinds of introspection/reflection features offered by SQLAlchemy.
- SQLAlchemy inspector
- Schema-supported reflection
- CrateDialect-specific reflection methods
Some examples from the upstream documentation at runtime inspection API, SQLAlchemy inspector, and Schema-supported reflection, have been paired with CrateDB-specific responses.
The result is a reasonably concise overview about them
>>> inspector.get_schema_names()
['blob', 'doc', 'information_schema', 'pg_catalog', 'sys']
>>> inspector.default_schema_name
'doc'
>>> dialect.has_schema(connection, 'doc')
True
Other than this, some parts of the documentation section within SQLAlchemy: Create, retrieve, update, and delete are also just vanilla SQLAlchemy operations, but I think the trio of getting-started.rst
, crud.rst
, and inspection-reflection.rst
still make sense in the context of the CrateDB driver, both as documentation, as well as a doctest scenario. Now that they have been refactored, I think they are in a better shape than before.
This section of the documentation outlines how to use CrateDB's SQLAlchemy | ||
integration. It demonstrates both basic database operations (insert, select, | ||
delete), creating and dropping tables, running queries with aggregations, | ||
as well as the use of complex and geospatial data types. |
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.
This sounds similar to the crud document. Would it make sense to merge the two into one? Or instead clarify the difference and link from one to the other.
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 for your attention. You are right. I've tried to improve this now with 5dc0e04.
The previous version was a bit of a potpourri of different topics. From that mix, two new documents have been created. One is about "advanced querying", including exercises about aggregations and full-text capabilities, and the other one exercises operations on the CrateDB container and geospatial special types concisely, now also consistently using the Object
and ObjectArray
type aliases, instead of the funny-sounding Craty
type.
I will be happy about any kinds of suggestions on improving the wording/tone, like your previous excellent patches.
…ctions Co-authored-by: Mathias Fußenegger <[email protected]>
55cac25
to
6090d53
Compare
The previous version was a bit of a potpourri of different topics. From that mix, two new documents have been created. One is about "advanced querying", including aggregations and full-text capabilities, and the other one exercises operations on the CrateDB container and geospatial special types concisely.
6090d53
to
5dc0e04
Compare
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.
Left some more suggestions. The new structure looks much better 👍
CrateDB currently does not support all variants as it can not handle the | ||
sub-queries yet. |
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.
Is this still the case?
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 question. I also wanted to highlight this note while reading it, but missed to do so. Which suggestion do you have?
a) Investigate the matter.
b) Just remove this sentence.
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.
a) + create an issue in the crate repo to support it if there are variants which it can't handle
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.
I will do. However, to move on with this, and other subsequent patches, I've deferred it to GH-489. I hope this is fine.
Aggregates: Counting and grouping | ||
================================= |
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.
Maybe move this elsewhere - the previous section is "Introduction to fulltext indexes" and the next section is "Fulltext search with MATCH predicate" - having an intermediate aggregation/count/group-by breaks the flow
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.
I agree and will try to find a different spot.
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.
The section has been moved one slot to the bottom with fe90300.
Co-authored-by: Mathias Fußenegger <[email protected]>
5f38b43
to
653c387
Compare
The "Aggregates: Counting and grouping" section breaks the reading flow, in between the full-text topic, which should not be separated.
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.
Thank you very much for your excellent contributions to this patch, @hammerhead and @mfussenegger. Merging it now.
About
This is a stab at #448, to improve the shape of doctests vs. rendered documentation. It moves some doctest files around into the documentation directory, and dissolves others into pure-Python tests.
New documentation section
The doctest files referenced below are now becoming first citizens of the public documentation, within a dedicated "by example" section. To make sure they are always valid, they are still evaluated as doctests. Many thanks to the authors who originally conceived them.
After adding some introductory paragraphs to the individual files, I think the new content is sweet already, and will be even better after another iteration of more content-related edits and refactorings by a subsequent patch.
Here, the new documentation section is rendered as a preview on RTD, see CrateDB Python client by example.
Details
doctests/{client,http,blob}.txt
anddoctests/{connection,cursor,https}.txt
have been moved to the documentation, to be rendered as reStructuredText (10da69c, 1731cc5).doctests/sqlalchemy.rst
andsqlalchemy/doctests/{dialect,itests,reflection}.txt
(13ed96f), which are now located atdocs/by-example/sqlalchemy/{cru,getting-started,inspection-reflection}.rst
.doctests/layer.txt
anddoctests/mocking.txt
are now implemented as regular Python tests.test.globs
, in order not to obfuscate them away from the users reading the examples, who are likely aiming to just copy/paste the presented code snippets.