Add BlockDataMock support for TrapDoor

Open spscally opened this issue 1 year ago • 7 comments

Description

Add BlockDataMock support for TrapDoor. Also:

moved BlockData String keys into consts class BlockDataKey to reduce duplication
refactored BlockDataMock.mock() since existing implementation would get rather large if more Tag-based subclasses are added

Checklist

The following items should be checked before the pull request can be merged.

[x] Unit tests added.
[ ] Code follows existing code format.

Info on creating a pull request

Make sure that unit tests are added which test the relevant changes.
Make sure that the changes follow the existing code format.

For maintainer

When a PR is approved, the maintainer should label this PR with release/*.

If the PR fixes a bug in MockBukkit, update the patch version. (if the current version is 0.4.1, the new version should be 0.4.2)
If the PR adds a new feature to MockBukkit, update minor version. (if the current version is 0.4.1, the new version should be 0.5.0)

Note that a PR that fixes an UnimplementedOperationException should be considered a new version and not a bugfix.

Oct 02 '22 18:10 spscally

Please fix the Codesmells and try to deduplicate the Code if possible

Oct 02 '22 21:10 thelooter

Please fix the Codesmells and try to deduplicate the Code if possible

Can do

Oct 02 '22 23:10 spscally

I've been thinking about how to deduplicate the code and I'm not sure there's a way of doing so without rewriting BlockDataMock. Here's why I think that:

Sonar is complaining about how getHalf()/setHalf()/getFacing()/setFacing()/getFaces() are duplicated in TrapDoorMock. If we wanted to put the BlockFace methods in a separate class, we could have an abstract class DirectionalMock that extends BlockDataMock and implements Directional (though getFaces() will have to differ for different subclasses). StairsMock, for example, could then extend DirectionalMock and that would reduce the duplication of those methods. However, when we also try dedupe the Half methods, we'd need something like BisectedMock, and we'd run into a multiple inheritance problem.

We could rewrite BlockDataMock to be a subinterface of BlockData and then allow for "mixing in" of Directional/Bisected/Waterlogged/etc methods, but I don't think I'm comfortable (nor do I think y'all would be) doing that having just started looking at the source code last week.

What are your thoughts? Is the duplication something you'd like for me to take a deeper look into?

(As for the code smells, I don't think there should be a problem fixing those.)

Oct 04 '22 12:10 spscally

It's something that would be nice to have done, but not a huge priority. If you're interested in it you can take a shot at it, but it's not required to keep adding new block data.

Oct 04 '22 23:10 Insprill

I addressed the smells. Also, I added jacocoTestReport to the test task, as I don't think it was running on test automatically.

Oct 05 '22 12:10 spscally

It's something that would be nice to have done, but not a huge priority. If you're interested in it you can take a shot at it, but it's not required to keep adding new block data.

Let me sit and think on that. I might be interested in trying my hand at it, but as of right now I don't have any ideas.

Oct 05 '22 12:10 spscally

Kudos, SonarCloud Quality Gate passed!