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

Explicitly treat ObjectDescription._doc_field_type_map as an instance variable #6482

Conversation

ViktorHaag
Copy link
Contributor

@ViktorHaag ViktorHaag commented Jun 13, 2019

Subject: Aims to address issue #6478

Feature or Bugfix

  • Bugfix

Purpose

Explicitly treat ObjectDescription._doc_field_type_map population as an instance variable so that we can be sure that Domains with objects that define their own field types will render properly on the output.

Detail

  • 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.

  • Ensured that ObjectDescription.get_field_type_map() made sure to populate the instance scoped version of _doc_field_type_map and not the class scoped version of the variable.

Relates

… 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.
@codecov
Copy link

codecov bot commented Jun 13, 2019

Codecov Report

Merging #6482 into 2.1.2 will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##            2.1.2    #6482      +/-   ##
==========================================
+ Coverage   83.65%   83.68%   +0.02%     
==========================================
  Files         280      278       -2     
  Lines       40478    40445      -33     
  Branches     5968     5965       -3     
==========================================
- Hits        33862    33846      -16     
+ Misses       5292     5277      -15     
+ Partials     1324     1322       -2
Impacted Files Coverage Δ
sphinx/directives/__init__.py 89.6% <100%> (-0.73%) ⬇️
sphinx/highlighting.py 86.23% <0%> (ø) ⬆️
sphinx/__init__.py
sphinx/make_mode.py
sphinx/util/docfields.py 88.94% <0%> (+0.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b307fc...e9ee39d. Read the comment docs.

- type annotation for _process_type_map was incorrect; needed to document the
  positional argument for the method

- __init__ method for ObjectDescription missing type annotation, so added
Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits.

sphinx/directives/__init__.py Outdated Show resolved Hide resolved
…ariable

- We can more simply have ObjectDescription.get_field_type_map() make sure that
  it's going to lazily set the value of an instance variable, not the class
  variable, when it populates the field type map.
Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for quick work!

@tk0miya tk0miya merged commit 982f63e into sphinx-doc:2.1.2 Jun 18, 2019
@tk0miya
Copy link
Member

tk0miya commented Jun 18, 2019

Merged. Thank you for your work!

tk0miya added a commit that referenced this pull request Jun 19, 2019
@ViktorHaag ViktorHaag deleted the object-descr-setting-class-type-map-iss6478 branch June 28, 2019 18:35
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants