Conversation
heevasti
left a comment
heevasti
left a comment
There was a problem hiding this comment.
Thanks for the edits and comments. Here back to you with review comments. Feel free to resolve or change at your discretion.
| The ``▻`` symbol denotes a reference to a named section of PEP-8. | ||
| Note that we only cite those sections where we deviate. | ||
|
|
||
| .. It might be worth it to use the headers to link to the relevant sections of PEP-8. |
There was a problem hiding this comment.
Yes, that does help to compare with the PEP-8 rules.
|
|
||
| .. It might be worth it to use the headers to link to the relevant sections of PEP-8. | ||
|
|
||
| .. Also, it seems that the arrow symbol is just used in every section here, even if it does not reference PEP-8. Examples of this are the logger naming conventions and asserts. |
There was a problem hiding this comment.
I think the points where the arrow symbol do not reference to PEP-8 should get replaced with something else or removed.
| ▻ Comments: Inline Comments | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| .. This does not deviate from PEP-8, so I don't think it should be here. |
There was a problem hiding this comment.
Then this can be removed.
| * Define the QMI version and check the Python version running QMI. | ||
| * Setup logging if the ``QMI_DEBUG`` environment variable is set (as True). Otherwise start at ``qmi.start()``. | ||
|
|
||
| .. A previous page said that any value for QMI_DEBUG would do, so I am now not sure which is correct. |
There was a problem hiding this comment.
I think this one '(as True)' is wrong, actually. If QMI_DEBUG is set, any value that does not evaluate as None will make the debugger logging enabled. So basically anything except null or "" (empty value) should activate it. Maybe some other very special character/case as well?
Best to remove here the '(as True)' part, and change in design.rst it to 'By setting environment variable QMI_DEBUG, at ...'. What do you think?
| But please avoid using the ``__all__ = [<Classes>]`` statement | ||
| to avoid enabling the ``from xxx import *`` import statements. | ||
|
|
||
| .. "from xxx import *" is not disabled if `__all__` is not set: it will simply import everything in the module. |
There was a problem hiding this comment.
Ah, you're right. If you set __all__ = [] THEN the from xxx import * does not import anything. If xxx here is a package. Maybe this should be used to restrict import namespaces for this private vs public modules?
Well, now they can use the asterisk import as __all__ is not there, so this is our default behaviour. Maybe we can change it later but for now let's just fix the text that says otherwise.
|
|
||
| ### Installing for generating documentation | ||
|
|
||
| [//]: # (As we do not list any other optional installations, I wonder why we have decided to list this specifically?) |
There was a problem hiding this comment.
Well, if someone wants to build the docs locally... It is OK to have?
|
|
||
| To build the 'readthedocs' documentation locally do: | ||
|
|
||
| [//]: # (This also does not work: documentation/sphinx/make-docs does not exist. I think this should say something about scripts/run_docs_sphinx.sh.) |
There was a problem hiding this comment.
Yes you're right. This is still how it is done with the Gitlab version of QMI. Looks like it didn't get updated for the Github. Please do update as you propose.
|
|
||
| ## Steps | ||
|
|
||
| [//]: # (Should this mention that you need to have bump2version installed through the [dev] dependencies?) |
There was a problem hiding this comment.
It's an option, but no need to mention it I think.
| Unit tests | ||
| ---------- | ||
|
|
||
| [//]: # (Should this mention Python 3.14?) |
There was a problem hiding this comment.
Right, since the latest release we support also 3.14. I forgot to edit that here.
| tests). | ||
| - Unit-tests are performed and the coverage is calculated. | ||
|
|
||
| [//]: # (Should this also run/mention Python 3.14?) |
2 participants
This PR contains the requested feedback on all non-generated documentation in the repo.
There are two kinds of changes in this PR:
Thus, this PR should not be merged without removal of the comments, as should not be merged into main.