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

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

Explicitly treat ObjectDescription._doc_field_type_map as an instance…
… variable

- Aims to address [issue #6478](#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

This comment has been minimized.

Copy link

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.

ViktorHaag added some commits Jun 13, 2019

mypy type annotations for methods added
- 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
@tk0miya
Copy link
Member

left a comment

LGTM with nits.

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

@tk0miya tk0miya added this to the 2.1.2 milestone Jun 15, 2019

Simplify to ensure we're lazily setting the instance field-type-map v…
…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.
sphinx/directives/__init__.py Outdated Show resolved Hide resolved
@tk0miya
Copy link
Member

left a comment

Thank you for quick work!

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

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tk0miya

This comment has been minimized.

Copy link
Member

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 ViktorHaag:object-descr-setting-class-type-map-iss6478 branch Jun 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.