crate / crate-python

Python DB API client library for CrateDB, using HTTP.
https://cratedb.com/docs/python/
Apache License 2.0
78 stars 31 forks source link

Improve doctests vs. rendered documentation #448

Closed amotl closed 5 months ago

amotl commented 2 years ago

Hi there,

this is a small proposal I would like to make. Please upvote if you think it should be improved, or downvote if you think it should be left in the current state. Comments are also welcome.

With kind regards, Andreas.

Introduction

At https://github.com/crate/crate-python/pull/442#discussion_r957012464, we had a conversation about the src/crate/client/doctests/cursor.txt file. On the other hand, there is also docs/query.rst. Both have in common that they demonstrate how to query data from CrateDB.

Problem

Apparently, cursor.txt is being tested ^1, but query.rst is not. Vice versa, query.rst is part of the documentation ^2, but cursor.txt is not.

Proposal

We may think about having both: Add query.rst to the test suite, and add cursor.txt to the rendered documentation. This will both improve code quality (query.rst will be validated) and the documentation (parts of the tests in cursor.txt may be interesting to readers).

amotl commented 2 years ago

@mfussenegger stated at https://github.com/crate/crate-python/pull/442#discussion_r991056478:

We shouldn't write doctests to ensure the client functions. Doctests are to ensure examples in the documention work. The primary concern is the documentation, not the test coverage it gives. If we use this for testing, we should convert it to regular python tests.

I absolutely agree with this. I only had the feeling that the use of doctests for testing was actually intended. I will be happy to steward the test suite into this direction.

amotl commented 2 years ago

Apparently, cursor.txt is being tested, but query.rst is not. Vice versa, query.rst is part of the documentation, but cursor.txt is not.

Coming from this situation, and aiming to implement your suggestion, I think the best way forward would be to:

amotl commented 2 years ago

Hi. My proposal to steward this into a better shape, is #464, still WIP. Early comments are welcome.

amotl commented 1 year ago

Status

I think #464 is ready now. On this page, the new documentation section is rendered as a preview on RTD:

Backlog

amotl commented 5 months ago

On behalf of this patch, the situation has been improved significantly already.

One backlog item to carry over:

Closing this.