Skip to content

Conversation

@henrikmidtiby
Copy link
Contributor

Overview: What does this pull request change?

Motivation and Explanation: Why and how do your changes improve the library?

Links to added or changed documentation pages

Further Information and Comments

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

@github-project-automation github-project-automation bot moved this to 🆕 New in Dev Board Dec 1, 2025
@henrikmidtiby henrikmidtiby added the typehints For adding/discussing typehints label Dec 1, 2025
@henrikmidtiby henrikmidtiby marked this pull request as ready for review December 1, 2025 22:27
@henrikmidtiby henrikmidtiby mentioned this pull request Dec 2, 2025
22 tasks
Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

I left suggestions on the code. Most of them are minor changes, but there's a specific suggestion which is probably controversial and might require some discussion: making the checkerboard_colors None instead of False. Maybe it's better to simply use Literal[False] in this PR, discuss the topic, and then possibly change it to None in a follow-up PR?

@github-project-automation github-project-automation bot moved this from 🆕 New to 👀 In review in Dev Board Dec 4, 2025
Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

I left suggestions on the code. Most of them are minor changes, but there's a specific suggestion which is probably controversial and might require some discussion: making the checkerboard_colors None instead of False. Maybe it's better to simply use Literal[False] in this PR, discuss the topic, and then possibly change it to None in a follow-up PR?

@henrikmidtiby henrikmidtiby linked an issue Dec 5, 2025 that may be closed by this pull request
22 tasks
@henrikmidtiby henrikmidtiby removed a link to an issue Dec 5, 2025
22 tasks
The opacity of the :class:`Surface`, from 0 being fully transparent
to 1 being fully opaque. Defaults to 1.
checkerboard_colors
ng individual faces alternating colors. Overrides ``fill_color``.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder what word is missing here ...

@henrikmidtiby
Copy link
Contributor Author

Maybe it's better to simply use Literal[False] in this PR, discuss the topic, and then possibly change it to None in a follow-up PR?

I would prefer that approach. The changes are quite easy to make (see d5c54cf), but it would be nice to discuss this among the developers.

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

Labels

typehints For adding/discussing typehints

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

2 participants