Skip to content

Conversation

myiume
Copy link
Contributor

@myiume myiume commented Sep 10, 2025

Added Optional to BlockPlaceEventEntry block var, so it can be triggered with any block

Summary by CodeRabbit

  • New Features

    • Block Place event now supports leaving the block type unset, allowing triggers on any block placement.
  • Improvements

    • More flexible block filtering: specify a material to restrict the event, or leave it blank to match all blocks.
    • Configuration is clearer and avoids unintended defaults (no longer defaults to Stone).

Copy link
Contributor

coderabbitai bot commented Sep 10, 2025

Walkthrough

Updated 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

Cohort / File(s) Summary
Block place event optional filter
extensions/BasicExtension/src/main/kotlin/com/typewritermc/basic/entries/event/BlockPlaceEventEntry.kt
Changed block from Material (default STONE) to Optional<Material> (default empty). Annotation updated from @MaterialProperties(BLOCK) to @MaterialProperties(MaterialProperty.BLOCK). Event check updated from direct equality to optional predicate, defaulting to allow when empty.

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
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Added Optional to BlockPlaceEventEntry block var" accurately reflects the primary change in the diff—making BlockPlaceEventEntry.block an Optional—so it is directly related to the changeset and clear to reviewers. It is concise and specific enough for a teammate scanning PRs to understand the main intent. The wording is slightly informal (e.g., "block var") but does not obscure the change.

Poem

I nudge the stones with twitchy grace,
Now any block may take its place—
Optional paths, a lighter hop,
If none is set, I will not stop.
Carrot in paw, I sift and see,
A freer field for blocks—and me! 🥕🐇

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.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

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 Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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-proofing

Use 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

📥 Commits

Reviewing files that changed from the base of the PR and between cb5b91d and 1215b05.

📒 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.

@gabber235
Copy link
Owner

@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.

@gabber235 gabber235 closed this Sep 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants