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

Fix issue where mixing different types of conditionals sometimes isn't allowed #5872

Merged
merged 11 commits into from
Aug 19, 2023
Merged

Fix issue where mixing different types of conditionals sometimes isn't allowed #5872

merged 11 commits into from
Aug 19, 2023

Conversation

Pikachu920
Copy link
Member

@Pikachu920 Pikachu920 commented Aug 1, 2023 •
edited
Loading

Description

The original implementation of multiline conditionals had some issues validating where a then section could be placed. This PR fixes those issues, adds some more comments to SecConditional and adds tests to ensure it works in the future.
image

Target Minecraft Versions: any
Requirements: n/a
Related Issues: #5866

@Pikachu920 Pikachu920 added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. 2.7 Targeting a 2.7.X version release labels Aug 1, 2023
@Pikachu920 Pikachu920 force-pushed the fix/mixed-conditionals branch from 884fcf8 to afd557a Compare August 1, 2023 04:34
APickledWalrus
APickledWalrus requested changes Aug 1, 2023
View reviewed changes
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

looks good, i think these changes have also made the code easier to follow.

just a 🤏 few small things

src/main/java/ch/njol/skript/sections/SecConditional.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/sections/SecConditional.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/sections/SecConditional.java Outdated Show resolved Hide resolved
Moderocky
Moderocky approved these changes Aug 19, 2023
View reviewed changes
Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

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

Look good :)

@APickledWalrus APickledWalrus merged commit 639bcf7 into master Aug 19, 2023
@APickledWalrus APickledWalrus deleted the fix/mixed-conditionals branch August 19, 2023 19:28
TheLimeGlass pushed a commit that referenced this pull request Aug 21, 2023
@Pikachu920 @TheLimeGlass
Fix issue where mixing different types of conditionals sometimes isn'…
8adf003 
…t allowed (#5872)

(cherry picked from commit 639bcf7)
@TheLimeGlass TheLimeGlass mentioned this pull request Aug 21, 2023
Merged
Moderocky pushed a commit that referenced this pull request Aug 22, 2023
@TheLimeGlass @Pikachu920
[2.7.0 target] Fix issue where mixing different types of conditionals (…
f32ac2c 
…#5932)

Fix issue where mixing different types of conditionals sometimes isn't allowed (#5872)

(cherry picked from commit 639bcf7)

Co-authored-by: Pikachu920 <28607612+Pikachu920@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@APickledWalrus APickledWalrus APickledWalrus approved these changes

@Moderocky Moderocky Moderocky approved these changes

@AyhamAl-Ali AyhamAl-Ali Awaiting requested review from AyhamAl-Ali

Assignees
No one assigned
Labels
2.7 Targeting a 2.7.X version release bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Pikachu920 @Moderocky @APickledWalrus @AyhamAl-Ali