Skip to content

Defer imports from font_manager in font module#368

Merged
rahulporuri merged 1 commit into
masterfrom
dev/defer-font-manager-imports
Sep 24, 2019
Merged

Defer imports from font_manager in font module#368
rahulporuri merged 1 commit into
masterfrom
dev/defer-font-manager-imports

Conversation

@rahulporuri
Copy link
Copy Markdown
Contributor

@rahulporuri rahulporuri commented Sep 24, 2019

Deferring these imports means that a number of places where we need to import from font don't trigger the import time side-effect in the font_manager module.

This PR doesn't attempt to actually fix either of the issues #362 or #363 .

Deferring this imports means that a number of places where we need
to import from font don't trigger the import time side-effect in
the font_manager module

	modified:   kiva/fonttools/font.py
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #368 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #368      +/-   ##
==========================================
+ Coverage   32.91%   32.91%   +<.01%     
==========================================
  Files         206      206              
  Lines       18303    18304       +1     
  Branches     2415     2415              
==========================================
+ Hits         6024     6025       +1     
  Misses      11893    11893              
  Partials      386      386
Impacted Files Coverage Δ
kiva/fonttools/font.py 56.79% <100%> (+0.54%) ⬆️

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 a843b37...04a084b. Read the comment docs.

@jwiggins
Copy link
Copy Markdown
Member

What is this fixing, if I may ask?

@rahulporuri
Copy link
Copy Markdown
Contributor Author

this PR should fix this issue - enthought/chaco#448

@jwiggins
Copy link
Copy Markdown
Member

Which references #362 and #363, neither of which are fixed here. Right?

@rahulporuri
Copy link
Copy Markdown
Contributor Author

Which references #362 and #363, neither of which are fixed here. Right?

Yes. The actual issues i.e. import-time side effects of font_manager and inability to load font files are not addressed here.

This PR simply aims to defer the import-time side effects to when the objects are actually needed. Deferring the imports prevents the tracebacks/logs from appearing when kiva or kiva.fonttools.font.Font are used.

For example, at the moment, simply doing import kiva will cause the tracebacks to appear because kiva.__init__ imports from kiva.fonttools.font and kiva.fonttools.font imports from kiva.fonttools.font_manager.

There are also a few places where Font is instantiated by the relevant findfont or findfontname methods are not used. Those uses also now don't need to worry about seeing this import-time side effect.

Does that answer your question?

Copy link
Copy Markdown
Member

@jwiggins jwiggins left a comment

Choose a reason for hiding this comment

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

Yes, I think that's enough context.

@rahulporuri rahulporuri merged commit 8ad2279 into master Sep 24, 2019
@rahulporuri rahulporuri deleted the dev/defer-font-manager-imports branch September 24, 2019 14:12
@rahulporuri
Copy link
Copy Markdown
Contributor Author

Thanks for the quick reviews John!

jwiggins pushed a commit that referenced this pull request Sep 30, 2019
Defer imports from font_manager in font module
@jwiggins jwiggins mentioned this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants