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.

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

🛠 Fix Duplicated Spaces in StructCommand #5416

Merged

Conversation

AyhamAl-Ali
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali commented Jan 30, 2023

Description

StructCommand Regex did not support duplicated spaces between command and / which causes command /test to throw an ISE (IllegalStateException)
This PR fixes it by supporting multiple spaces


Target Minecraft Versions:
Requirements:
Related Issues:

@AyhamAl-Ali AyhamAl-Ali added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Jan 30, 2023
@AyhamAl-Ali AyhamAl-Ali requested a review from TPGamesNL January 31, 2023 11:54
@AyhamAl-Ali AyhamAl-Ali added the 2.7 Targeting a 2.7.X version release label Feb 1, 2023
Copy link
Member

@TPGamesNL TPGamesNL 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 regex pattern change is good (do separate the fields onto multiple lines :)

I think we shouldn't assume that the matcher (line 186) returns true, cause they're different parsers. I think it'd be better to return false when there's no match (maybe error too)

@TheLimeGlass TheLimeGlass self-requested a review February 2, 2023 05:25
@AyhamAl-Ali AyhamAl-Ali requested a review from TPGamesNL February 2, 2023 20:26
@TheLimeGlass TheLimeGlass removed the 2.7 Targeting a 2.7.X version release label Mar 8, 2023
@AyhamAl-Ali AyhamAl-Ali added the 2.7 Targeting a 2.7.X version release label Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants