Skip to content

Conversation

@Touchie771
Copy link
Owner

fixed #8

Copy link
Owner Author

@Touchie771 Touchie771 left a comment

Choose a reason for hiding this comment

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

The PR is on the right track for fixing issue #8, but needs additional changes before it's ready to merge.

@Touchie771
Copy link
Owner Author

Review of PR #12

The PR is on the right track for fixing issue #8, but needs these additional changes before merging:

Required Changes:

  1. README.md examples - Remove all (short) casts from the code examples:

    • Line 54: (short) 1313
    • Line 75: (short) 1010
    • Line 76: (short) 1313
    • Line 77: (short) 1616
    • Line 83: (short) 2222
  2. README.md API documentation (line 127):
    Change short itemSlotint itemSlot

Suggested Improvements:

  1. Consider dynamic validation in Menu.java:
    The hardcoded 0-53 range assumes chest inventories. Consider making this dynamic based on the actual inventory size.

  2. Add a migration note in the PR description since this is a breaking API change.

Once these are addressed, the PR will be ready to merge! 🎉

@Touchie771
Copy link
Owner Author

Perfect! All requested changes have been implemented.

This PR now fully addresses issue #8:

  • ✅ Changed itemSlot type from short to int in SlotItem record
  • ✅ Updated legacy constructor parameter type
  • ✅ Removed all (short) casts from README examples (lines 54, 75-77, 83)
  • ✅ Updated API documentation from short itemSlot to int itemSlot (line 127)

The breaking change is well-documented and the API is now more intuitive. Using int for slot positions makes more sense than short.

Ready to merge! 🎉

@Touchie771 Touchie771 merged commit e99aa11 into master Nov 27, 2025
2 checks passed
@Touchie771 Touchie771 deleted the bad-short-design branch November 27, 2025 18:14
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.

Inappropriate use of short for slot positions

2 participants