Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adding "indirectly redstone powered" condition #3297

Merged
merged 12 commits into from
Aug 28, 2020
Merged

Adding "indirectly redstone powered" condition #3297

merged 12 commits into from
Aug 28, 2020

Conversation

Romitou
Copy link
Member

@Romitou Romitou commented Aug 11, 2020

Description

This Pull Request brings the addition of the "indirectly redstone powered" condition.
I directly added this to the existing condition. If you think it was a bad idea and that another condition should have been made, let me know!

Target Minecraft Versions: any
Requirements: none
Related Issues: #3294

Whimsyturtle
Whimsyturtle reviewed Aug 11, 2020
View reviewed changes
Copy link
Member

@Whimsyturtle Whimsyturtle left a comment

Choose a reason for hiding this comment

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

Just some comments!

src/main/java/ch/njol/skript/conditions/CondIsBlockRedstonePowered.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsBlockRedstonePowered.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsBlockRedstonePowered.java Outdated Show resolved Hide resolved
@Whimsyturtle Whimsyturtle added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Aug 11, 2020
@Romitou @FranKusmiruk
CondIsBlockRedstonePowered- Addition of suggested changes
0715578 
Co-authored-by: Frank Kusmiruk <48283695+FranKusmiruk@users.noreply.github.com>
Whimsyturtle
Whimsyturtle reviewed Aug 12, 2020
View reviewed changes
Copy link
Member

@Whimsyturtle Whimsyturtle left a comment

Choose a reason for hiding this comment

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

Other than that, looks good to merge.

src/main/java/ch/njol/skript/conditions/CondIsBlockRedstonePowered.java Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsBlockRedstonePowered.java Show resolved Hide resolved
Pikachu920
Pikachu920 previously approved these changes Aug 12, 2020
View reviewed changes
Copy link
Member

@Pikachu920 Pikachu920 left a comment

Choose a reason for hiding this comment

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

code looks good - only question I have is: are we sure isBlockPowered returns false when isIndirectlyPowered is true? if it doesn't, directly shouldn't be part of the pattern

@Pikachu920 Pikachu920 self-requested a review August 13, 2020 14:57
@Romitou
Copy link
Member Author

Romitou commented Aug 13, 2020 •
edited
Loading

Thank you for your code comments. From what I have tested, I can confirm that isBlockPowered returns false when isBlockIndirectlyPowered is true on some blocks, if your question was about that.

@Pikachu920
Copy link
Member

is isBlockPowered and isBlockIndirectlyPowered ever true at the same time?

@Romitou
Copy link
Member Author

Romitou commented Aug 13, 2020

Oops, I didn't get it that way. So yes, isBlockPowered and isBlockIndirectlyPowered can be true at the same time. In this case, what shouldn't be part of the pattern?

@Whimsyturtle
Copy link
Member

Oops, I didn't get it that way. So yes, isBlockPowered and isBlockIndirectlyPowered can be true at the same time. In this case, what shouldn't be part of the pattern?

If that's the case, then the [directly] part of the condition pattern should be removed, my bad

@FranKusmiruk FranKusmiruk added this to the 2.6 milestone Aug 18, 2020
@FranKusmiruk FranKusmiruk linked an issue Aug 18, 2020 that may be closed by this pull request
Add a condition, Block#isBlockIndirectlyPowered() #3294
Closed
@ShaneBeee ShaneBeee merged commit dcd89af into SkriptLang:master Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@APickledWalrus APickledWalrus APickledWalrus left review comments

@Pikachu920 Pikachu920 Pikachu920 approved these changes

@Whimsyturtle Whimsyturtle Whimsyturtle approved these changes

@FranKusmiruk FranKusmiruk FranKusmiruk left review comments

Assignees
No one assigned
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Milestone
2.6
Development

Successfully merging this pull request may close these issues.

Add a condition, Block#isBlockIndirectlyPowered()
6 participants
@Romitou @Pikachu920 @Whimsyturtle @APickledWalrus @FranKusmiruk @ShaneBeee