Add more system info #3641
Add more system info #3641
Conversation
📝 WalkthroughWalkthroughTwo new system query functions are added to the pygame system module: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/system_test.py (2)
8-11: Consider adding a bounds check for CPU core count.The test correctly verifies the return type. To make it more robust, you could also assert that the value is positive, since modern systems should have at least one CPU core.
Apply this diff to add a bounds check:
def test_get_cpu_cores(self): cpu = pygame.system.get_cpu_cores() self.assertIsInstance(cpu, int) + self.assertGreater(cpu, 0, "CPU core count should be positive")
25-28: Consider verifying the platform string is non-empty.The test correctly verifies the return type. You could optionally add an assertion that the string is non-empty, since SDL should always return a platform name.
Apply this diff to add a non-empty check:
def test_get_platform(self): platform = pygame.system.get_platform() self.assertIsInstance(platform, str) + self.assertTrue(len(platform) > 0, "Platform string should not be empty")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src_c/doc/system_doc.his excluded by!src_c/doc/*
📒 Files selected for processing (3)
buildconfig/stubs/pygame/system.pyi(2 hunks)src_c/system.c(3 hunks)test/system_test.py(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:115-126
Timestamp: 2025-09-01T20:22:31.010Z
Learning: In pygame-ce PR #3573, the PG_SURF_BitsPerPixel function implementation uses hardcoded return values of 32 for YUY2/UYVY/YVYU FOURCC formats because it's copying the exact logic from SDL's internal SDL_GetMasksForPixelFormat function. This ensures perfect compatibility between SDL2 and SDL3 behaviors rather than using the technically more accurate values that might be returned by SDL_GetPixelFormatDetails.
📚 Learning: 2025-09-01T20:22:31.010Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:115-126
Timestamp: 2025-09-01T20:22:31.010Z
Learning: In pygame-ce PR #3573, the PG_SURF_BitsPerPixel function implementation uses hardcoded return values of 32 for YUY2/UYVY/YVYU FOURCC formats because it's copying the exact logic from SDL's internal SDL_GetMasksForPixelFormat function. This ensures perfect compatibility between SDL2 and SDL3 behaviors rather than using the technically more accurate values that might be returned by SDL_GetPixelFormatDetails.
Applied to files:
src_c/system.c
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Applied to files:
src_c/system.c
🧬 Code graph analysis (1)
test/system_test.py (2)
src_c/system.c (1)
system(283-328)buildconfig/stubs/pygame/system.pyi (2)
get_cpu_cores(30-39)get_platform(89-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: aarch64
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Pyodide build
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (macos-14)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.10.17)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: dev-check
🔇 Additional comments (5)
buildconfig/stubs/pygame/system.pyi (2)
30-39: LGTM! Clear documentation with helpful note about hyperthreading.The type stub is well-structured with appropriate type annotations and comprehensive documentation. The note about logical vs. physical cores is a valuable clarification for users.
89-106: LGTM! Excellent documentation with helpful examples and notes.The type stub is well-crafted with clear examples of supported platforms and useful notes about the "Unknown" fallback behavior and the comparison with the
platformlibrary.src_c/system.c (3)
7-11: LGTM! Implementation is correct and consistent with existing patterns.The function correctly wraps
SDL_GetCPUCount()and properly handles the conversion to a Python integer. IfPyLong_FromLong()fails, it returns NULL which correctly propagates the exception. This pattern is consistent withpg_system_get_total_ramin the same file.
72-76: LGTM! Clean implementation with proper SDL integration.The function correctly wraps
SDL_GetPlatform()and converts the result to a Python string. Since SDL_GetPlatform() never returns NULL (it returns "Unknown" for unrecognized platforms), andPyUnicode_FromString()automatically handles errors by returning NULL, the error handling is appropriate.
267-268: LGTM! Method table entries are correctly configured.Both method table entries properly use
METH_NOARGSsince the functions take no parameters, and reference the appropriate documentation constants. The configuration is consistent with other entries in the method table.Also applies to: 273-274
aatle
added
New API
ankith26
left a comment
ankith26
left a comment
There was a problem hiding this comment.
Sorry, while I appreciate your efforts, I'm -1 on this PR.
I don't see why we should re-implement equivalents of os.cpu_count() and sys.platform on our side.
|
I agree with Ankith. |
4 participants
This PR adds get_cpu_cores and get_platform functions to the system module, providing basic system information to reduce the need for external libraries.