Log non-compliant animated stickers #302
Conversation
rob93c
added
unit test
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (3)
src/test/java/com/github/stickerifier/stickerify/media/MediaHelperTest.java (1)
242-247: LGTM! Consider enhancing error message verification.The test follows the established pattern for negative test cases and integrates well with the existing test suite. However, we could strengthen the test by verifying the specific exception message.
Consider enhancing the assertion to verify the exception message:
- assertThrows(MediaException.class, () -> MediaHelper.convert(archive)); + var exception = assertThrows(MediaException.class, () -> MediaHelper.convert(archive)); + assertThat(exception.getMessage(), containsString("non-compliant animated sticker"));src/main/java/com/github/stickerifier/stickerify/media/MediaHelper.java (2)
150-158: LGTM! Consider adding units for clarity.The toString() implementation provides a clear representation of the animation details. However, consider adding units to make the output more explicit:
- "width=" + width + - ", height=" + height + - ", frameRate=" + frameRate + - ", duration=" + duration() + + "width=" + width + "px" + + ", height=" + height + "px" + + ", frameRate=" + frameRate + "fps" + + ", duration=" + duration() + "s" +
169-179: Enhance logging with specific compliance failures.While the current logging is helpful, it would be more useful to log which specific requirements weren't met. This would help users understand exactly why their sticker was rejected.
boolean isCompliant = animation != null && animation.frameRate() <= MAX_ANIMATION_FRAME_RATE && animation.duration() <= MAX_ANIMATION_DURATION_SECONDS && animation.width() == MAX_SIDE_LENGTH && animation.height() == MAX_SIDE_LENGTH; if (!isCompliant) { - LOGGER.atWarn().log("The {} doesn't meet Telegram's requirements", animation); + if (animation == null) { + LOGGER.atWarn().log("Invalid animation: null"); + return false; + } + if (animation.frameRate() > MAX_ANIMATION_FRAME_RATE) { + LOGGER.atWarn().log("The {} exceeds maximum frame rate of {}", animation, MAX_ANIMATION_FRAME_RATE); + } + if (animation.duration() > MAX_ANIMATION_DURATION_SECONDS) { + LOGGER.atWarn().log("The {} exceeds maximum duration of {}s", animation, MAX_ANIMATION_DURATION_SECONDS); + } + if (animation.width() != MAX_SIDE_LENGTH || animation.height() != MAX_SIDE_LENGTH) { + LOGGER.atWarn().log("The {} has invalid dimensions. Required: {}x{}", animation, MAX_SIDE_LENGTH, MAX_SIDE_LENGTH); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
src/test/resources/non_compliant_animated_sticker.gzis excluded by!**/*.gz
📒 Files selected for processing (2)
src/main/java/com/github/stickerifier/stickerify/media/MediaHelper.java(2 hunks)src/test/java/com/github/stickerifier/stickerify/media/MediaHelperTest.java(1 hunks)
rob93c
force-pushed
the
log-non-compliant-animated-stickers
branch
from
9224ec1 to
0693fc7
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Tests