brechtm / rinohtype

The Python document processor
http://www.mos6581.org/rinohtype
GNU Affero General Public License v3.0
498 stars 59 forks source link

Print alt text instead of error message when failing to render rst :image: directive #410

Closed Timm638 closed 1 year ago

Timm638 commented 1 year ago

I've written this request as a proof of concept for my suggestion #409. In this implementation, there is no possibility to disable that behaviour. I don't know how to connect that with a template option.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

brechtm commented 1 year ago

Thanks, this is a useful change. The patch looks good to me. I just added some comments with my thoughts.

I'm thinking it could make sense to omit including the error in the PDF altogether and only print it to the terminal on rendering? Oops, I was confused there. That's what your patch does if the alt text is supplied. 👍

brechtm commented 1 year ago

In this implementation, there is no possibility to disable that behaviour. I don't know how to connect that with a template option.

I don't think this needs to be configurable. That can always be added later in case someone comes up with a use case for it.

codecov[bot] commented 1 year ago

Codecov Report

Merging #410 (38a1877) into master (a9268d6) will decrease coverage by 0.01%. The diff coverage is 22.22%.

@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
- Coverage   46.89%   46.88%   -0.01%     
==========================================
  Files          93       93              
  Lines       14665    14671       +6     
  Branches     2409     2410       +1     
==========================================
+ Hits         6877     6879       +2     
- Misses       7618     7622       +4     
  Partials      170      170              
Flag Coverage Δ
3.10 46.89% <22.22%> (-0.01%) :arrow_down:
3.11 46.89% <22.22%> (-0.01%) :arrow_down:
3.12.0-alpha ?
3.7 45.72% <22.22%> (-0.01%) :arrow_down:
3.8 46.88% <22.22%> (-0.01%) :arrow_down:
3.9 46.88% <22.22%> (-0.01%) :arrow_down:
Linux 46.88% <22.22%> (-0.01%) :arrow_down:
pypy-3.9 46.90% <22.22%> (-0.01%) :arrow_down:
unit-3.10 46.89% <22.22%> (-0.01%) :arrow_down:
unit-3.11 46.89% <22.22%> (-0.01%) :arrow_down:
unit-3.12 ?
unit-3.7 45.72% <22.22%> (-0.01%) :arrow_down:
unit-3.8 46.88% <22.22%> (-0.01%) :arrow_down:
unit-3.9 46.88% <22.22%> (-0.01%) :arrow_down:
unit-pypy3 46.90% <22.22%> (-0.01%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/rinoh/frontend/rst/nodes.py 51.68% <0.00%> (-0.11%) :arrow_down:
src/rinoh/image.py 67.63% <16.66%> (-0.72%) :arrow_down:
src/rinoh/stylesheets/matcher.py 100.00% <100.00%> (ø)
brechtm commented 1 year ago

Sorry this is taking so long to process. I don't have much time for rinohtype lately, unfortunately...

Several regressions tests are failing due to these changes. Can you look into those? This is one of the issues:

TypeError: InlineImage.__init__() got an unexpected keyword argument 'alt'
brechtm commented 1 year ago

/rebase

brechtm commented 1 year ago

Merged. Thanks, @Timm638!