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

Allow nullable patterns with different modifier orders. #6591

Merged
merged 6 commits into from
Apr 19, 2024

Conversation

Moderocky
Copy link
Member

Description

SkriptParser checked for nullable/default inputs to start with -, which would fail if the input was something like %~-objects% (since it starts with ~, not -).

I delegated it to a method that checks the first n = min(length, 5) characters of the string for the - and fails if one isn't present, or the text of the input (like objects) starts before the - is found.

Obviously, if we add a hundred new %!?^&*$-~+objects modifiers in future this limit will need to be increased, but I kept it low for safety in case of some unusual (addon) pattern.


Target Minecraft Versions: any
Requirements: none
Related Issues: closes #6202

@Moderocky Moderocky added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. 2.8 Targeting a 2.8.X version release labels Apr 19, 2024
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.

I think we could come up with a better solution. Perhaps we could find a way to integrate TypePatternElement here and do away with this string loops that searches for expressions:

If we don't wish to change that much at this time, we should at least just call getExprInfo earlier and check exprInfo.isOptional (less changes, but arguably a worse solution)

@sovdeeth
Copy link
Member

I think we could come up with a better solution. Perhaps we could find a way to integrate TypePatternElement here and do away with this string loops that searches for expressions:

If we don't wish to change that much at this time, we should at least just call getExprInfo earlier and check exprInfo.isOptional (less changes, but arguably a worse solution)

Wouldn't it be better to just loop in both cases? TypePatternElement is pretty hacky in that it checks for - twice, once before and once after checking for ~/*. Shouldn't we have a method that will loop over all the modifiers in the pattern and set their fields all at once instead of having to write out the entire loop manually like we currently do?

Right now it's check -, check ~*, check - again, but if we add X, now it's check -, check ~*, check - again, check X, check - a third time, check ~* again, check - a fourth time

@APickledWalrus
Copy link
Member

That check can be improved, but we should prefer to use the already compiled patterns rather than the substring stuff going on here

@sovdeeth
Copy link
Member

That check can be improved, but we should prefer to use the already compiled patterns rather than the substring stuff going on here

yeah makes sense

@Moderocky
Copy link
Member Author

I've decided to fix both at the same time.

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. Just to note, Kenzie and I discussed the usage of TypePatternElement and that will be targeted for another PR.

@@ -264,7 +267,7 @@ else if ((flagMask & SkriptParser.PARSE_EXPRESSIONS) == 0)
return stringBuilder.append("%").toString();
}

private ExprInfo getExprInfo() {
public ExprInfo getExprInfo() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public ExprInfo getExprInfo() {
private ExprInfo getExprInfo() {

We can expose this in a future PR if necessary

@Moderocky Moderocky merged commit 7140f82 into SkriptLang:dev/patch Apr 19, 2024
4 checks passed
@Moderocky Moderocky deleted the fix-6202 branch April 19, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 Targeting a 2.8.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.

4 participants