Skip to content

Conversation

@hugovk
Copy link
Member

@hugovk hugovk commented Feb 29, 2024

Changes proposed in this pull request:

  • Move global __main__ calls into main() to avoid shadowing variables
  • Refactor version numbers into constants to reduce duplication and make updating easier

See also the last commit, I first put them as separate constants like:

BROTLI_VERSION = "1.1.0" FREETYPE_VERSION = "2.13.2" ...

I then refactored that into a single dict:

V = { "BROTLI": "1.1.0", "FREETYPE": "2.13.2", ....

So we can refer to them like V['BROTLI'] instead of BROTLI_VERSION, but I'm not sure which I prefer.

Thoughts?

cc @nulano

Copy link
Contributor

@nulano nulano left a comment

Choose a reason for hiding this comment

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

Looks good.

Two suggestions:

  • Do we also want V['libimagequant'] for consistency?
  • I would also include SF_PROJECTS in the format strings now that it's not the only variable. IIRC it was done with a plus operator because it was the same length, but now it saves one character :).
Co-authored-by: Ondrej Baranovič <ondreko.tiba@gmail.com>
@hugovk
Copy link
Member Author

hugovk commented Mar 3, 2024

Two suggestions:

  • Do we also want V['libimagequant'] for consistency?

Yes, but let's do it when we're no longer pointing to a Git hash:

"libimagequant": {
# commit: Merge branch 'master' into msvc (matches 2.17.0 tag)
"url": "https://github.com/ImageOptim/libimagequant/archive/e4c1334be0eff290af5e2b4155057c2953a313ab.zip",
"filename": "libimagequant-e4c1334be0eff290af5e2b4155057c2953a313ab.zip",
"dir": "libimagequant-e4c1334be0eff290af5e2b4155057c2953a313ab",
"license": "COPYRIGHT",
"patch": {
"CMakeLists.txt": {
"if(OPENMP_FOUND)": "if(false)",
"install": "#install",
# libimagequant does not detect MSVC x86_arm64 cross-compiler correctly
"if(${{CMAKE_SYSTEM_PROCESSOR}} STREQUAL ARM64)": "if({architecture} STREQUAL ARM64)", # noqa: E501
}
},
"build": [
*cmds_cmake("imagequant_a"),
cmd_copy("imagequant_a.lib", "imagequant.lib"),
],
"headers": [r"*.h"],
"libs": [r"imagequant.lib"],
},

  • I would also include SF_PROJECTS in the format strings now that it's not the only variable. IIRC it was done with a plus operator because it was the same length, but now it saves one character :).

Good idea, suggestions applied!

@hugovk
Copy link
Member Author

hugovk commented Mar 3, 2024

I've added a couple more commits to silence IDE (PyCharm) squiggly warnings.

Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@radarhere radarhere merged commit 3f5721d into python-pillow:main Mar 9, 2024
@hugovk hugovk deleted the refactor-winbuild branch March 9, 2024 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

3 participants