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

ObjectDescription doc field type map incorrectly set #6478

Closed
ViktorHaag opened this issue Jun 12, 2019 · 3 comments
Closed

ObjectDescription doc field type map incorrectly set #6478

ViktorHaag opened this issue Jun 12, 2019 · 3 comments

Comments

@ViktorHaag
Copy link
Contributor

ViktorHaag commented Jun 12, 2019

Describe the bug
Refactoring the setting of an ObjectDescription class's type map out of the DocFieldTransformer and into the ObjectDescription class itself wasn't done correctly, I think. Typed fields built into an object description that's part of a domain aren't always parsed and rendered correctly.

To Reproduce
Start with a fresh Sphinx 2.1.1 install, and install sphinxcontrib.httpdomain. Then, create a brand new sphinx project, and make two changes:

Change the project's conf.py so that:

extensions = [ 'sphinxcontrib.httpdomain' ]

Change the project's index.rst so that you first add a simple HTTP Domain documentation block to the bottom:

.. http:get:: /users/(id)

   Retrieve a user record by ID.

   :param string id: User identifier.
   :query email: Include email address.
   :qtype email: boolean

When you now make html the project, you will see that the output for the docs is as you expect; you see the syntax of the typed query and qtype field pair produce a field entry that reads:

Query Parameters:  * email (boolean) -- Include email address.

Now add a documentation block from a different domain just above the http:get block you already have:

.. py:function:: foo(id) -> string

   Create a foo.

   :param string id: Identifier.


.. http:get:: /users/(id)

   Retrieve a user record by ID.

   :param string id: User identifier.
   :query email: Include email address.
   :qtype email: boolean

When you make clean html now you will notice that Sphinx has stopped properly processing the field syntax for the fields under the http:get entry; the output looks like this now:

Query email: Include email address.
Qtype email: boolean

Note that if you add the python documentation block after the HTTP get block, and you use a typed field that isn't already defined in the HTTP domain's list of field types, then it's the Python documentation that gets effected:

.. http:get:: /users/(id)

   Retrieve a user record by ID.

   :param string id: User identifier.
   :query email: Include email address.
   :qtype email: boolean

.. py:function:: foo(id) -> string

   Create a foo.

   :param string id: Identifier.
   :variable bar: Bar variable.
   :vartype bar: string

With this arrangement, the output is like this:

GET /users/(id)
  Retrieve a user record by ID.

  Parameters:  * id (string) – User identifier.
  Query Parameters:  * email (boolean) – Include email address.


foo(id) → string
  Create a foo.

  Parameters:  * id (string) – Identifier.
  Variable bar:  Bar variable.
  Vtype bar:  string

For me, this raises the urgency of this bug; it's not just confined to a single domain extension (like sphinxcontrib-httpdomain), but to all (?) HTML documentation built where you have more than one domain used in the documentation where several children classes of ObjectDescription get used (i.e., most documentation projects of any size or complexity).

Expected behavior
I expect that the type fields for all domains will render properly, based on the doc fields type map for their individual domains.

Environment info

  • OS: MacOS
  • Python version: 3.7.3
  • Sphinx version: 2.1.1
  • Sphinx extensions: sphinxcontrib.httpdomain (but I also believe this will happen with any two domains that have classes that inherit from ObjectDescription)

Additional context
I believe that this might be related to not preserving the behaviour that used to exist in DocFieldTransformer by lazily populating the _doc_field_type_map on the class of the directive passed into it.

Here is what the code for the doc field transformer constructor used to look like:

def __init__ (self, directive):
       self.directive = directive
       if '_doc_field_type_map' not in directive.__class__.__dict__:
            directive.__class__._doc_field_type_map = \
                self.preprocess_fieldtypes(directive.__class__.doc_field_types)
        self.typemap = directive._doc_field_type_map
 

I believe that this was careful to operate on the _doc_field_type_map variable on the class of the directive passed in, and not the instance. Now we have this in ObjectDescription instead:

    _doc_field_type_map = {}  # type: Dict[str, Tuple[Field, bool]]

    def get_field_type_map(self):
        # type: () -> Dict[str, Tuple[Field, bool]]
        if self._doc_field_type_map == {}:
            for field in self.doc_field_types:
                for name in field.names:
                    self._doc_field_type_map[name] = (field, False)

                if field.is_typed:
                    typed_field = cast(TypedField, field)
                    for name in typed_field.typenames:
                        self._doc_field_type_map[name] = (field, True)

        return self._doc_field_type_map

Notice that this operates on self._doc_field_type_map instead, and only if it hasn't yet been populated.

I believe that this means that any class inheriting from ObjectDescription that doesn't explicitly declare a _doc_field_type_map of its own will not get the results expected; it will have some other class's doc field type map already set, and so it won't be able to set it's own, and so what you see is that additional typed fields that are declared (like the "query and qtype" fields in HTTResource) don't get properly set, and so when processing those fields when more than one domain is in play, they don't get properly parsed.

I haven't found a way to get around this without declaring a _doc_field_type_name variable in the HTTPResource inheriting from ObjectDescription (in sphinxcontrib.httpdomain), and I don't think that's a good way to fix this, because then every inheriting class from ObjectDesciption presumably needs its own such variable.

Note that I tried changing ObjectDescription.get_field_type_map() to work on self.__class__._doc_field_type_map instead, but that doesn't seem to work, so there's probably something else going on here that I'm not seeing how to fix properly.

@ViktorHaag
Copy link
Contributor Author

Note that issue #6474 and PR #6192 seem related ...

ViktorHaag added a commit to ViktorHaag/sphinx that referenced this issue Jun 13, 2019
… variable

- Aims to address [issue sphinx-doc#6478](sphinx-doc#6478)

- Left ObjectDescription._doc_field_type_map declaration alone and in place in
  case there are other factors at play that require it declared on the class.

- In ObjectDescription constructor, explicitly set _doc_field_type_map's value
  as an instance variable.

- This presumably needs to be built with every instantiation of an
  ObjectDescription or inheriting class, so we're not attempting to "define it
  once on a class and then use it"... that approach seems much harder to reason
  about and get correctly done (as was maybe demonstrated by the refactoring
  that lead to this problem in the linked-to issue).

- Runs through the test suite clean locally except for an unrelated single test
  failure on an image_converter test, which also failed on the base branch
  before my changes here.
@ViktorHaag
Copy link
Contributor Author

As far as I can determine from local testing, #6482 addresses this, however, the solution might not be something you want as a general approach.

@tk0miya tk0miya added the domain label Jun 15, 2019
@tk0miya tk0miya added this to the 2.1.2 milestone Jun 15, 2019
@ViktorHaag
Copy link
Contributor Author

See also: #6488

@tk0miya tk0miya closed this as completed Jun 18, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants