GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Use different colour for 'Return value: Borrowed reference' by hugovk · Pull Request #188 · python/python-docs-theme

Conversation

@hugovk

@hugovk hugovk commented Apr 17, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Follow on from python/cpython#117926.

Currently light green is used for all return types:

  • "Return value: Always NULL."
  • "Return value: New reference."
  • "Return value: Borrowed reference."

For example: https://docs.python.org/3/c-api/exceptions.html

image

image

image

A borrowed reference is not necessarily a "bad" thing, but we may want to use something other than green that indicates it's "good", to indicate they should be treated with care.

This PR uses the same yellow/orange colour as the "Changed in version x.y" text in #185 - I've already checked the contrast ratios in that PR and both meet WCAG AAA. By using the same colour, it will be easier to check/change when we audit.

image image

For example: https://python-docs-theme-previews--188.org.readthedocs.build/en/188/c-api/exceptions.html#querying-the-error-indicator

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@hugovk
Use different colour for 'Return value: Borrowed reference'
ade62ae
@hugovk hugovk mentioned this pull request Apr 17, 2024
Docs: Add classes to C API return value annotations python/cpython#117926
Merged
@erlend-aasland
erlend-aasland requested review from encukou, erlend-aasland and vstinner April 17, 2024 18:48
@vstinner

vstinner commented Apr 17, 2024

Copy link
Copy Markdown
Member

This PR uses the same yellow/orange colour as the "Changed in version x.y" text in #185

FYI As a color blind person, I don't see the difference between "Borrowed reference" and "New reference" colors in the light mode. Well, I'm also color blind, so my opinion may not matter :-)

In the dark mode, I see clearly that the two colors are very different.

erlend-aasland and hugovk reacted with heart emoji
All reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@flying-sheep

flying-sheep commented Apr 19, 2024

Copy link
Copy Markdown

If you’re reusing existing colors, how about you centralize their definition as a CSS variables and then use it from both your new CSS rules and the old ones that originally defined the color?

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hugovk

hugovk commented Apr 19, 2024

Copy link
Copy Markdown
Member Author

Good idea, shall we also replace the pale green for the other "Return value" with the "New in version" green?

image

The light mode ones are much closer, but we could replace one with the other here too?

image

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hugovk
Refactor version change colours into variables, also use for refcounts
8b3ec0b
@hugovk

hugovk commented Apr 24, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Updated:

image

image

image

image

Suggestions for better variable names welcome :)

humitos, erlend-aasland, and flying-sheep reacted with heart emoji
All reactions
  • ❤️ 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland
erlend-aasland approved these changes Apr 24, 2024
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't have a strong opinion regarding naming, but I like the change! Personally, I would prefer borrowed references to be labelled as "bad", but that is a different discussion.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hugovk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@hugovk
hugovk merged commit 97952b5 into python:main Apr 26, 2024
@hugovk
hugovk deleted the refcount-return-borrowed-ref branch April 26, 2024 10:26
@hugovk

hugovk commented Jun 26, 2024

Copy link
Copy Markdown
Member Author

This is now deployed: https://docs.python.org/3/c-api/exceptions.html

flying-sheep and erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland approved these changes
@encukou encukou Awaiting requested review from encukou
@vstinner vstinner Awaiting requested review from vstinner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@hugovk @vstinner @flying-sheep @erlend-aasland
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.