-
-
Notifications
You must be signed in to change notification settings - Fork 96
Added Optional to BlockPlaceEventEntry block var #385
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.
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
Conversation
WalkthroughUpdated BlockPlaceEventEntry to make the block filter optional: the block property now uses Optional with a default empty value, adjusted annotations to MaterialProperty.BLOCK, and updated the event handler to allow all blocks when the optional is empty. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Player
participant Bukkit as Bukkit Event System
participant Handler as BlockPlaceEvent Handler
participant Entry as BlockPlaceEventEntry
Player->>Bukkit: Place block
Bukkit->>Handler: BlockPlaceEvent
Handler->>Entry: Read entry.block (Optional<Material>)
alt entry.block is present
Handler->>Handler: Compare event.block.type == entry.block
alt match
Handler-->>Bukkit: Proceed with entry logic
else no match
Handler-->>Bukkit: Skip entry logic
end
else entry.block is empty
note over Handler,Entry: No constraint → allow by default
Handler-->>Bukkit: Proceed with entry logic
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/event/BlockPlaceEventEntry.kt (1)
55-56
: Prefer event.blockPlaced for clarity/future-proofingUse the explicit “placed block” accessor; it’s self-documenting and avoids API ambiguities across Bukkit/Paper versions.
- entry.block.map { it == event.block.type }.orElse(true) + entry.block.map { it == event.blockPlaced.type }.orElse(true)Additionally (outside this hunk) update the earlier location read for consistency:
val position = event.blockPlaced.location.toPosition()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/event/BlockPlaceEventEntry.kt
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
extensions/*Extension/**
📄 CodeRabbit inference engine (extensions/AGENTS.md)
When adding a new extension, create its project directory with a name ending in "Extension" (e.g., MyPluginExtension)
Files:
extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/event/BlockPlaceEventEntry.kt
extensions/{*Extension/src/**/*.kt,*Extension/**/*.kts}
📄 CodeRabbit inference engine (extensions/AGENTS.md)
Use 4 spaces for indentation and wrap lines at 120 characters in Kotlin code and Gradle Kotlin scripts
Files:
extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/event/BlockPlaceEventEntry.kt
extensions/*Extension/src/**/*.kt
📄 CodeRabbit inference engine (extensions/AGENTS.md)
extensions/*Extension/src/**/*.kt
: Keep functions short and focused; prefer guard clauses over nested conditionals
Document public APIs with KDoc, focusing on when to use them rather than restating method behavior
Avoid inline comments; refactor unclear code instead of adding explanatory comments
Files:
extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/event/BlockPlaceEventEntry.kt
🧠 Learnings (1)
📚 Learning: 2025-08-24T11:10:07.264Z
Learnt from: CR
PR: gabber235/Typewriter#0
File: .github/instructions/entity-extension.add-entity.instructions.md:0-0
Timestamp: 2025-08-24T11:10:07.264Z
Learning: Applies to extensions/EntityExtension/src/main/kotlin/com/typewritermc/entity/entries/entity/minecraft/*Entity.kt : Ensure imports/types compile: Ref, Var, ConstVar, OnlyTags, and correct colors/icons/description in Entry
Applied to files:
extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/event/BlockPlaceEventEntry.kt
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (1)
extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/event/BlockPlaceEventEntry.kt (1)
35-36
: Do not revert the default to STONE — Optional.empty() is intentional and consistent
- Optional.empty() is used by other event entries (e.g., extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/event/BlockBreakEventEntry.kt) to mean "match any"; reverting to Material.STONE would be a behavioral change not required here.
- Nit: the static import of MaterialProperty.BLOCK is unused — either use @MaterialProperties(BLOCK) or remove the import in extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/event/BlockPlaceEventEntry.kt.
Likely an incorrect or invalid review comment.
@myiume I have bad news for you: this can’t be merged. Merging it would basically break everyone’s setup. In v1.0, I plan to create a proper way to migrate entries, along with many other changes. The issue is that the expected data shape will change, but the underlying data can’t be migrated yet, so it breaks the setup. |
Added Optional to BlockPlaceEventEntry block var, so it can be triggered with any block
Summary by CodeRabbit
New Features
Improvements