Skip to content

Add _normalize_coords() to reduce duplicate code #9899

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.

Sign up for GitHub

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

Open
radarhere wants to merge 1 commit into
base: main
Choose a base branch
from
+12 −12
Open

Add _normalize_coords() to reduce duplicate code #9899

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions src/PIL/ImageDraw.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,16 @@ def circle(
ellipse_xy = (xy[0] - radius, xy[1] - radius, xy[0] + radius, xy[1] + radius)
self.ellipse(ellipse_xy, fill, outline, width)

def _normalize_coords(self, xy: Coords) -> Sequence[Sequence[float]]:

@akx akx Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should maybe be a free function in the module?

@radarhere radarhere Aug 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Why? It's not used anywhere else.

@akx akx Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since it doesn't need to be overridden by any subclass of ImageDraw I can conceive of, and free functions are a tiny bit faster to call:

In the disassembly for a quick timing example,

  • a free function call is LOAD_GLOBAL (looks at the module globals table) + LOAD_FAST_BORROW for the argument + CALL
  • an instance function call is LOAD_FAST_BORROW for the local self + LOAD_ATTR (a dict lookup on the instance) + LOAD_FAST_BORROW for the argument + CALL

So in other words, because it's unnecessary to be an instance method.

@radarhere radarhere Aug 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

My personal preference would be for code that is only called from internal methods in the instance to be a method, as an indicator of how it is used. It also places it much closer to the first self._normalize_coords(xy), making it quicker to understand.

"""Normalize 1 or 2 dimensional coord sequence into 2d sequence."""
if isinstance(xy[0], (list, tuple)):
return cast("Sequence[Sequence[float]]", xy)
else:
return [
cast("Sequence[float]", tuple(xy[i : i + 2]))
for i in range(0, len(xy), 2)
]

def line(
self,
xy: Coords,
Expand All @@ -234,14 +244,7 @@ def line(
if ink is not None and width != 0:
self.draw.draw_lines(xy, ink, width)
if joint == "curve" and width > 4:
points: Sequence[Sequence[float]]
if isinstance(xy[0], (list, tuple)):
points = cast("Sequence[Sequence[float]]", xy)
else:
points = [
cast("Sequence[float]", tuple(xy[i : i + 2]))
for i in range(0, len(xy), 2)
]
points = self._normalize_coords(xy)
for i in range(1, len(points) - 1):
point = points[i]
angles = [
Expand Down Expand Up @@ -397,10 +400,7 @@ def rounded_rectangle(
corners: tuple[bool, bool, bool, bool] | None = None,
) -> None:
"""Draw a rounded rectangle."""
if isinstance(xy[0], (list, tuple)):
(x0, y0), (x1, y1) = cast("Sequence[Sequence[float]]", xy)
else:
x0, y0, x1, y1 = cast("Sequence[float]", xy)
(x0, y0), (x1, y1) = self._normalize_coords(xy)
if x1 < x0:
msg = "x1 must be greater than or equal to x0"
raise ValueError(msg)
Expand Down