Skip to content

Add blocks interfaces for commons properties #6639

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

Open
wants to merge 7 commits into
base: minor-next
Choose a base branch
from

Conversation

Dhaiven
Copy link
Contributor

@Dhaiven Dhaiven commented Feb 21, 2025

Related issues & PRs

Add #6161

Changes

API changes

Add:

  • AgeableInterface
  • AnalogRedstoneSignalEmitterInterface
  • ColoredInterface
  • CoralTypeInterface
  • LightableInterface
  • MultiFacingInterface
  • PillarRotationInterface
  • PoweredByRedstoneInterface
  • SignLikeRotationInterface
  • SingleFacingInterface
  • WoodTypeInterface

Behavioural changes

Backwards compatibility

No BC Break

Follow-up

Tests

@Dhaiven Dhaiven requested a review from a team as a code owner February 21, 2025 13:11
Copy link
Member

@dktapps dktapps left a comment

Choose a reason for hiding this comment

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

I think the names of the interfaces are too verbose. I understand why (e.g. WoodType would conflict with the WoodType enum), but I don't like it much.

@dktapps dktapps added Category: API Related to the plugin API Type: Enhancement Contributes features or other improvements to PocketMine-MP Status: Waiting on Author labels Feb 23, 2025
@Dhaiven
Copy link
Contributor Author

Dhaiven commented Feb 27, 2025

I think the names of the interfaces are too verbose. I understand why (e.g. WoodType would conflict with the WoodType enum), but I don't like it much.

May be replace Interface by Property (e.g. WoodTypeProperty, HorizontalFacingProperty...)

@ShockedPlot7560
Copy link
Member

I think the names of the interfaces are too verbose. I understand why (e.g. WoodType would conflict with the WoodType enum), but I don't like it much.

The aim is also to follow the name of the trait with the name of the corresponding interface, I think. If we were to rename these interfaces, it would probably make more sense to rename the trait accordingly.
Then, it might be possible to name IXxxx instead of XxxxInterface? Idk

@dktapps
Copy link
Member

dktapps commented Feb 28, 2025

I'd personally favour e.g. HasWoodType but I'm not sure about it.

@Dhaiven
Copy link
Contributor Author

Dhaiven commented Feb 28, 2025

I'd personally favour e.g. HasWoodType but I'm not sure about it.

In my opinion, syntax $block instanceof HasWoodType is not the best

@dktapps
Copy link
Member

dktapps commented Feb 28, 2025

I'd personally favour e.g. HasWoodType but I'm not sure about it.

In my opinion, syntax $block instanceof HasWoodType is not the best

yeah I consider that but I don't really like any of the alternatives

@dktapps
Copy link
Member

dktapps commented Feb 28, 2025

Perhaps for wood specifically we can follow copper's lead with WoodMaterial. I don't recall if there are any other potential conflicts.

@Dhaiven
Copy link
Contributor Author

Dhaiven commented Mar 2, 2025

Perhaps for wood specifically we can follow copper's lead with WoodMaterial. I don't recall if there are any other potential conflicts.

So you want i remove Interface for names and for wood i put WoodMaterial ?

@dktapps
Copy link
Member

dktapps commented Mar 2, 2025

Yeah, that's where I'm leaning right now. Welcome to suggest better alternatives.

Copy link
Member

@dktapps dktapps left a comment

Choose a reason for hiding this comment

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

The self returns need to be documented as @return $this

@ipad54
Copy link
Member

ipad54 commented Mar 12, 2025

Just my opinion, but what do you think about making facing interfaces to end on able word? Like MultiFaceable, HorizontalFaceable, SignLikeRotatable?

@dktapps
Copy link
Member

dktapps commented Mar 12, 2025

I don't have much opinion on that, aside from preferring consistency with the traits (since the traits and interfaces will typically be used together).

Copy link

This PR has been marked as "Waiting on Author", but we haven't seen any activity in 7 days.

If there is no further activity, it will be closed in 28 days.

Note for maintainers: Adding an assignee to the PR will prevent it from being marked as stale.

Comment on lines +36 to 38
class Wood extends Opaque implements PillarRotation{
use PillarRotationTrait;
use WoodTypeTrait;
Copy link
Member

Choose a reason for hiding this comment

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

WoodMaterial is missing

public function getOutputSignalStrength() : int;

/**
* @throws \InvalidArgumentException if `$signalStrength` is out of bounds
Copy link
Member

Choose a reason for hiding this comment

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

Should we really notice InvalidArgumentException ?

Copy link
Member

Choose a reason for hiding this comment

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

No, we typically don't document this since it's not supposed to be caught

Comment on lines +29 to +37
* @return int between 0 and 15
*/
public function getRotation() : int;

/**
* @param int $rotation between 0 and 15
*
* @throws \InvalidArgumentException if `$rotation` is out of hounds
* @return $this
Copy link
Member

Choose a reason for hiding this comment

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

If we give bounds here, we need to give it for the others

Copy link

This PR has been marked as "Waiting on Author", but we haven't seen any activity in 7 days.

If there is no further activity, it will be closed in 28 days.

Note for maintainers: Adding an assignee to the PR will prevent it from being marked as stale.

@github-actions github-actions bot added the Stale label Apr 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: API Related to the plugin API Stale Status: Waiting on Author Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants