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

gh-121035: Update logging flow chart to include lastResort #121036

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

bessman
Copy link
Contributor

@bessman bessman commented Jun 26, 2024

@bessman bessman requested a review from vsajip as a code owner June 26, 2024 08:57
Copy link

cpython-cla-bot bot commented Jun 26, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Jun 26, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bessman bessman force-pushed the doc/last-resort-logging-flow branch from ae0d1dd to ec93c98 Compare June 26, 2024 11:43
@vsajip
Copy link
Member

vsajip commented Jun 27, 2024

Thanks for this, but what process have you used? I've been thinking about replacing (or perhaps augmenting) this PNG file with something that's more easily modifiable, such as an SVG.

@bessman
Copy link
Contributor Author

bessman commented Jun 27, 2024

I edited it in GIMP. Not the most convenient workflow, indeed.

If you prefer, I could look into recreating the flowchart as SVG in Inkscape or Dia. It won't look exactly the same, of course, but I'm sure I can get it reasonably close.

@vsajip
Copy link
Member

vsajip commented Jun 27, 2024

I could look into recreating the flowchart as SVG

That would be great. I think it needs to be a bit "nicer", style-wise, than the current iteration, which looks pretty old-fashioned and out of step with the overall documentation that surrounds it. If you think that's too much work, I'll understand!

@bessman
Copy link
Contributor Author

bessman commented Jun 27, 2024

I think it needs to be a bit "nicer", style-wise

I'm not much of a graphic designer, so I can't really execute on this without further guidance. Do you have an example of an image in the style you would like?

For now, I have replaced the PNG with an SVG in the same style. I'll experiment a bit with font size and background to make sure it's readable.

@bessman
Copy link
Contributor Author

bessman commented Jun 27, 2024

I'm working on embedding the SVG in the page HTML properly, so that the font is the same as the surrounding page text.

@vsajip
Copy link
Member

vsajip commented Jun 27, 2024

Quick work, it's coming along nicely. Things that occurred to me:

  • I would change "lastResort" to "handler of last resort"
  • The overall size of the SVG might need to be tweaked, too - at least from what I can see from the documentation preview.
  • Maybe keep your updated PNG in the repository (for now), in case we find that some browsers can't display the SVG (unlikely, I know). We can link the SVG from the documentation as you've done, but keep the PNG around in case we need to add an additional link to that.

@bessman bessman force-pushed the doc/last-resort-logging-flow branch from efa0237 to fa836c7 Compare June 27, 2024 13:03
@bessman
Copy link
Contributor Author

bessman commented Jun 27, 2024

I would change "lastResort" to "handler of last resort"

How about "lastResort handler"? My thinking is that for someone unfamiliar with the logging module, it is immediately obvious that "lastResort" is the name of an actual Python object, and a quick ctrl+f on the same page will bring the user to an explanation of what it is. Whereas "handler of last resort" is less obvious what it is, at least to me.

@bessman
Copy link
Contributor Author

bessman commented Jun 27, 2024

One problem: The sphinx :class: invert-in-dark-mode command does not work with raw html. I've added a bit of CSS to the SVG which makes it aware of the browser's dark mode setting, but I haven't been able to figure out a way to make it respond to the theme selector drop down. I.e. if someone with their browser set to dark mode selects light mode from the theme selector, the diagram will look bad. Same for light mode browser + dark mode theme selector.

An alternative is to load the SVG with .. image:: instead of .. raw:: html, but then the text in the diagram won't be copyable or accessible to screen readers.

@vsajip
Copy link
Member

vsajip commented Jun 27, 2024

One problem: The sphinx :class: invert-in-dark-mode

Maybe leave that for a later refinement? Accessibility is probably more important than a light/dark mismatch. To react to the theme selector would probably need a bit of javaScript.

@bessman bessman force-pushed the doc/last-resort-logging-flow branch from 81841f9 to b1ce092 Compare June 27, 2024 20:10
@bessman
Copy link
Contributor Author

bessman commented Jun 27, 2024

Then I think this is ready.

@vsajip vsajip added the docs Documentation in the Doc dir label Jun 27, 2024
@vsajip vsajip merged commit 237baf4 into python:main Jun 27, 2024
28 checks passed
@vsajip
Copy link
Member

vsajip commented Jun 27, 2024

Thank you very much for this!

@vsajip vsajip added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Jun 27, 2024
@miss-islington-app
Copy link

Thanks @bessman for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @bessman for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 27, 2024
…handler. (pythonGH-121036)

(cherry picked from commit 237baf4)

Co-authored-by: Alexander Bessman <bessman@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 27, 2024
…handler. (pythonGH-121036)

(cherry picked from commit 237baf4)

Co-authored-by: Alexander Bessman <bessman@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented Jun 27, 2024

GH-121105 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Jun 27, 2024
@bedevere-app
Copy link

bedevere-app bot commented Jun 27, 2024

GH-121106 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 27, 2024
@bessman bessman deleted the doc/last-resort-logging-flow branch June 27, 2024 21:24
vsajip pushed a commit that referenced this pull request Jun 27, 2024
vsajip pushed a commit that referenced this pull request Jun 27, 2024
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
@vsajip
Copy link
Member

vsajip commented Jul 1, 2024

@bessman I made some changes to the SVG to tweak elements positions and styles. I want to export the changed version to PNG, but the PNG I export from Inkscape has a transparent background. What tool did you end up using to create the SVG and PNG (including precise versions, and on which OS)? I modified the SVG by hand as I couldn't find a suitable tool that would make minimal changes to the SVG you created.

@vsajip
Copy link
Member

vsajip commented Jul 1, 2024

There's still a pending issue of syncing with the theme selector, but I think that will require changes in the pydoc theme to e.g. add a class to the body element indicating the mode in use.

@bessman
Copy link
Contributor Author

bessman commented Jul 2, 2024

What tool

I made it in Dia (v0.97+git from Ubuntu 20.04). Perhaps the .dia file should also be added to the repo?

To create the PNG, I used Dia's export feature. However, after some experimentation I've found a way to do the conversion with imagemagick (6.9.10-23 Q16 x86_64 20190101) which achieves better compression:

convert logging_flow.svg -background none -define png:compression-filter=1 -define png:compression-level=9 -define png:compression-strategy=0 logging_flow.png

@vsajip
Copy link
Member

vsajip commented Jul 2, 2024

Thanks for the info.

Perhaps the .dia file should also be added to the repo?

I've not used Dia, is the .dia file text? We don't generally add non-standard formats to the repo. If Dia can import SVG, I suppose we might not need it?

BTW I think I've now addressed the theme selector issue in #121254. Feel free to try it out. It's merged in the main branch, but not yet backported to 3.12/3.13.

@bessman
Copy link
Contributor Author

bessman commented Jul 2, 2024

is the .dia file text?

It's gzip'd XML.

@vsajip
Copy link
Member

vsajip commented Jul 2, 2024

Sadly, I've noticed that following my changes (mostly to remove inline styles from the elements in favour of CSS), Dia won't import the SVG correctly any more. See this screenshot:

dia

@bessman
Copy link
Contributor Author

bessman commented Jul 2, 2024

Well, in case it's helpful, here is the .dia file: logging_flow.xml.gz

I had to change the suffix to .xml.gz for github to accept it.

@vsajip
Copy link
Member

vsajip commented Jul 2, 2024

Thanks very much.

noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants