AGENTS.md, Sphinx docstrings, section headers and pylint

[kulath]

There are a few cases where I think AGENTS.md has imposed 'rules' that are getting a bit ahead of our guidelines and conventions.

(Sorry, there doesn't appear to be a way to quote this part - oh dear, the markup for Discussions is terrible)

---

Docstrings

All functions and methods must have docstrings. Keep them concise — a short description is almost always enough:

def close(self):
    """
    Close the specified database.
    """

Use the full Sphinx format (:param:, :type:, :returns:, :rtype:) only when the parameters or return value are non-obvious and not already expressed by type hints:

def get_person_from_gramps_id(self, gramps_id: PersonGrampsID) -> Person | None:
    """
    Return a Person from the database using the given gramps ID.

    :param gramps_id: The gramps ID of the Person to retrieve.
    :type gramps_id: PersonGrampsID
    :returns: The Person object, or ``None`` if not found.
    :rtype: :py:class:`Person`
    """

Do not add verbose multi-line docstrings to simple functions, and never repeat information already expressed by the function name or type hints.

---

(1) The idea of concise docstrings seems to be derived from, and OK for simple interfaces like those mentioned (which are from database acces), but will not be OK for more complicated code that has significant business logic.

Just a random example from narrativeweb as I am familiar with that module:

def _build_obj_dict(self):
        Construct the dictionaries of objects to be included in the reports.
        There are two dictionaries, which have the same structure: they are two
        level dictionaries,the first key is the class of object
        (e.g. gen.lib.Person).
        The second key is the handle of the object.

        For the obj_dict, the value is a tuple containing the gramps_id,
        the text name for the object, and the file name for the display.

        For the bkref_dict, the value is a tuple containing the class of object
        and the handle for the object that refers to the 'key' object.

Of course, AI will tend to apply the rules literally. I think the rules are not always appropriate.

(2) I think that all functions and methods should have Sphinx style docstrings so that they can be extracted into the Gramps API documentation https://www.gramps-project.org/docs/. In the past, people have probably been quite lazy about writing documentation for the purpose that it would be extracted into this API docs, but now with the AI guides, we might be in the situation where AI (and human interpretation of the AI guides) will tend to limit completion of the documentation, at the same time as AI will actually make it much simpler to provide better documentation. (I know I tried getting AI to review and help with docs of an addon, and it was really helpful in providing Sphinx documentation).

I made a suggestion for #2279, but I think maybe it was too late to be fully considered, or needs some discussion.

---

Import Grouping

Imports must be organized into sections, each preceded by a comment header of this form:

# -------------------------------------------------------------------------
#
# Standard Python modules
#
# -------------------------------------------------------------------------
import os
import logging

# -------------------------------------------------------------------------
#
# GTK/Gnome modules
#
# -------------------------------------------------------------------------
from gi.repository import Gtk

# -------------------------------------------------------------------------
#
# Gramps modules
#
# -------------------------------------------------------------------------
from gramps.gen.db.base import DbReadBase
from .mymodule import MyClass

Not all sections are required — include only those that apply. Common section names used in the codebase include Standard Python modules, GTK/Gnome modules, and Gramps modules.

---

Again, this rule has forced AI to require all imports to conform to this rule. There are about 1280 .py files in the gramps repository (according to Google AI which may be rubbish). Of those, about 284 have a Python Modules comment and 376 have a Standard Python modules comment (case ignored). This has led to a PR #2284 suggesting some 1089 file changes. I don't think the programming guidelines https://gramps-project.org/wiki/index.php/Programming_guidelines#Imports actually suggests anything about these section headers for imports.

Do we really want to make that many changes, when some of the modules may have very few, or very simple imports - the style and taste of the writer may be better than an automatic AI suggestion. (Ref Guido and hobgoblins).

---

Inline suppression comments such as # pylint: disable=import-outside-toplevel are not acceptable.

---

I agree that pylint is really useful, and can catch some errors that AI misses. However:

Again, I think that this rule is too restrictive. As an example, I have an addon where an import is only needed if debug logging is turned on, so I have put it guarded by the relevant debug test later in the code with a pylint disable comment. Similarly, there is an import that will only be needed in an edge case, so it is guarded by that edge case with a disable comment. These are just my examples, you may disagree with them or maybe you would have written them differently - that is not the point. I can understand that pylint comments like too many local variables or too many parameters or too many branches should probably NOT be disabled, because one would want to see them in pylint output. The point is that the rule in AGENTS.md is just too strict.

[kulath]

I should add that the original change in #2279 said:

Notes` that inline # pylint: disable=... suppressions are acceptable when the author has a deliberate reason

but this seems to have been removed (though I can't see where this was done).

I should also add that the Sphinx parameter and result documentation not only covers type(-hints), but also the use, purpose and meaning of the parameters and results. What may be obvious to the designer and coder at the time they wrote the code may not be obvious to the reader of the documentation.

[dsblank]

@kulath are you able to create a PR with your suggestions? It is hard to tell from the text what is currently in the file, and what is you proposed fix.

[kulath]

@kulath are you able to create a PR with your suggestions? It is hard to tell from the text what is currently in the file, and what is you proposed fix.

No, sorry, but here is a diff that I think may contain my suggestions (but mangled by github). I haven't tested it with AI.

@@ -101,16 +101,7 @@

Docstrings

-All functions and methods must have docstrings. Keep them concise — a short description is almost always enough:

-```python
-def close(self):

• """
• Close the specified database.
• """
  -```

-Use the full Sphinx format (:param:, :type:, :returns:, :rtype:) only when the parameters or return value are non-obvious and not already expressed by type hints:
+All functions and method must have docstrings using the Sphinx format (:param:, :type:, :returns:, :rtype:) (so that documentation can be automatically extracted):

def get_person_from_gramps_id(self, gramps_id: PersonGrampsID) -> Person | None:
@@ -124,11 +115,11 @@
    """

-Do not add verbose multi-line docstrings to simple functions, and never repeat information already expressed by the function name or type hints.
+For simple functions or methods, a concise description is often enough as in the immediately preceding example. For more complicated functions or methods a longer description is appropriate. Where there is significant business logic, this may need to be outlined. Any special cases may need to be explained.

Import Grouping

-Imports must be organized into sections, each preceded by a comment header of this form:
+Imports should generally be organized into sections, each preceded by a comment header of this form:

# -------------------------------------------------------------------------

[kulath]

Maybe AGENTS needs to explicitly be told that reviews should not report on imports not being grouped and not having the exact right comment headers. It seems that this is just noise.

[stevenyoungs]

-Use the full Sphinx format (:param:, :type:, :returns:, :rtype:) only when the parameters or return value are non-obvious and not already expressed by type hints:
+All functions and method must have docstrings using the Sphinx format (:param:, :type:, :returns:, :rtype:) (so that documentation can be automatically extracted):

I'm neither for nor against this.
However if we do require docstrings for type-hinted methods, we should have a validation step in the PR workflow to ensure they remain consistent.

[Nick-Hall]

A few comments:

Docstrings for methods in the public API are used in our documentation. They should be as detailed as required to give a good description. Sphinx style should be used. Parameters and the return value should be included.

We generally group imports into 3 main sections: Python, GObject and Gramps. I don't think that an extra section for third-party python modules is very helpful. Imports can be further grouped if the developer thinks that it would be useful. Imports need not be in alphabetical order, a logical order is also acceptable and may be preferable in some cases.

The use of pylint overrides is not allowed.

[kulath]

A few comments:

Docstrings for methods in the public API are used in our documentation. They should be as detailed as required to give a good description. Sphinx style should be used. Parameters and the return value should be included.

Agreed - I have suggested AGENTS be changed to say "All functions and method must have docstrings using the Sphinx format (:param:, :type:, :returns:, :rtype:)"

We generally group imports into 3 main sections: Python, GObject and Gramps. I don't think that an extra section for third-party python modules is very helpful. Imports can be further grouped if the developer thinks that it would be useful. Imports need not be in alphabetical order, a logical order is also acceptable and may be preferable in some cases.

"Generally" is good (and I agree), assuming that AI interprets that as not forcing the import headers.

The use of pylint overrides is not allowed.

OK. (Not my choice, but I accept it). I think there are only about 84 occurrences of #pylint: disable in the main Gramps code, so not much to change if change is needed. (I don't know why gen/lib/ all have imports outside toplevel - seems unecessary.

The interesting question would be how many modules have pylint scores under 9, and is it reasonable to make this score a rule. I suspect that there are many modules that violate it and it may be too restrictive.

[Nick-Hall]

The interesting question would be how many modules have pylint scores under 9, and is it reasonable to make this score a rule. I suspect that there are many modules that violate it and it may be too restrictive.

The result of pylint reports should be used for guidance only. The aim of a score of 9 or above is a good indicator, but not an absolute rule. We don't want people cluttering the code with #pylint: disable just to meet this target.

A good example is the use of x, y and z as variables - generally a bad idea, but in code where they are cartesian coordinates - acceptable. No need to disable warnings.

Many smaller files probably don't meet the 9+ target, because the pylint score averages over the number of lines of code.

[emyoulation]

A question about Sphinx, docstrings and snippets:

The Gramps API documentation section Useful Snippets is virtually unpopulated.

How is a 'Useful Snippet' marked for indexing?

[Nick-Hall]

Useful snippets are defined in the doc/api.rst file

[kulath]

@Nick-Hall @dsblank on reflection, it would be much better to simply remove the whole “Imports Grouping” section from AGENTS.md, and just make sure it is in the Gramps Programming Guidelines in the Wiki.

The section is not at all necessary for AI agents to check, and is just causing so much noise (both in AI reviews, and in PRs and discussions), and in current code is more honoured in the breach than the observance, so it can’t really be important.

[iand]

What about: "When writing new code organise the imports as follows: Python, GObject, Gramps. When modifying existing code, follow place new imports into the appropriate group but don't reorder existing imports. Do not flag out of order imports when reviewing code."

[stevenyoungs]

The section is not at all necessary for AI agents to check

Agreed, but without it, an AI authored new file will have imports in whatever order it chooses. So I think we need to give AI some guidance.

More fundamentally, do the import section headers add enough value for the lines they consume, or could we just remove them all?
Does the grouping of imports help humans enough to require it?
I'm ambivalent on both questions.

[hgohel]

Does the grouping of imports help humans enough to require it?

This is really the key! In my years programming with C++, organizing alphabetically helped in several ways - one it avoided specific ordering to avoid errors and two, made it easy to find, insert and delete includes, avoiding duplicates. We used to break it up into standard includes, 3P includes and product includes. Finally, specifying a guideline made it so that large teams would follow them consistently over imposing their own preferences each time a file was checked in. So yes, I think grouping and alphabetizing can be helfpul.

[kulath]

As it happens, neither the Gramps programming guidelines, nor the current AGENTS.md mention alphabetical order, although this is the subject of several comments here.

New modules entirely authored by AI

The section is not at all necessary for AI agents to check

Agreed, but without it, an AI authored new file will have imports in whatever order it chooses. So I think we need to give AI some guidance.

I agree that it may be useful to give guidance to AI for new modules.

Existing modules

More fundamentally, do the import section headers add enough value for the lines they consume, or could we just remove them all? Does the grouping of imports help humans enough to require it? I'm ambivalent on both questions.

I don't think the import section headers generally add enough value, and in particular, for existing modules, there is no point in altering modules to comply, and we don't want noise from AI reviews about this. So I think AGENTS.md should make it clear that AI should not check existing code.

I have edited the Programming Guidelines to summarise what I think the current AGENTS.md and @Nick-Hall have suggested. So I hope this is a compromise to the guidance.

[kulath]

@iand I think it’s still too complicated and specific for AGENTS.md. Anyway AGENTS was flagging missing comment headers, rather than out of order imports, so this wouldn’t help for AGENTS.

I still prefer just removing the whole section from AGENTS. Much simpler.

The first couple of sentences might do for adding to the Programming Guidelines (possibly).