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 CondCompare Logging #6266

Merged

Conversation

APickledWalrus
Copy link
Member

Description

This fixes logging problems with CondCompare that were the cause of the issue below. The errors in the linked issue are ghost errors from CondCompare reparsing literals to find a good comparison. The problem is, even when CondCompare is successful, it prints the entire log which includes errors from failed Classes.parse attempts.

To combat this issue, I have replaced the RetainingLogHandler with a ParseLogHandler. When it needs to print an error, it will print the most recent/relevant error. At the end, when successful, it will print all non-error log messages (e.g. warnings that are likely to be applicable).


Target Minecraft Versions: any
Requirements: none
Related Issues:

@APickledWalrus APickledWalrus 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 Dec 29, 2023
Copy link
Contributor

@Fusezion Fusezion 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 to me and runs as intended, regression test should be added

Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

Tests would be beneficial indeed.

@sovdeeth sovdeeth added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Dec 30, 2023
@APickledWalrus
Copy link
Member Author

I'm not sure of any easy way to test this as the errors did not occur with a vanilla Skript installation. I think it will be alright without a test since the changes are minimal.

@APickledWalrus APickledWalrus merged commit 48e18f9 into SkriptLang:dev/feature Dec 30, 2023
4 checks passed
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. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants