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

Multilined Conditional + Order Debug Fix #7431

Open
wants to merge 5 commits into
base: dev/feature
Choose a base branch
from

Conversation

TheAbsolutionism
Copy link
Contributor

@TheAbsolutionism TheAbsolutionism commented Jan 14, 2025

Description

This PR aims to fix the skipped debugging of embedded multilined conditions such as SecConditional by debugging the conditions within init which is caught in the active RetainingLogHandler and is printed in the correct spot via my fix for the order.
Also fixes the order of debug messages due to Section.parse and handler.printlog within ScriptLoader#loadItems which was placing all embedded elements above the debugged line of the parent SectionNode.

Example Code:

on load:
	if all:
		1 is greater than 2
		3 is less than 5
	then:
		broadcast "HI"

	if true is false:
		broadcast "BYE"

Before:
idea64_e9iwt4cEYj

After:
idea64_nKL1Vxc3gK

Example Code from #7236

on break:
	if player's tool is a diamond sword:
		loop blocks in radius 3 around player:
			send "hi" to player

idea64_cXm1KWRQGV


Target Minecraft Versions: any
Requirements: none
Related Issues: #7236 , #7237

@APickledWalrus APickledWalrus self-requested a review January 14, 2025 05:48
@APickledWalrus
Copy link
Member

We should look into ways for the condition debug messages to be printed from the syntax classes themselves. The ScriptLoader shouldn't have knowledge of any specific syntax implementations

@TheAbsolutionism
Copy link
Contributor Author

TheAbsolutionism commented Jan 14, 2025

We should look into ways for the condition debug messages to be printed from the syntax classes themselves. The ScriptLoader shouldn't have knowledge of any specific syntax implementations

That was my original plan, however, when I tried it, adding newlines for each of the condition, obviously did not retain the indentation, and when I had tried (granted I may have done it wrong) but trying getParser().getIndentation() did not align correctly (I cant really remember from my attempt)

You say that it shouldn't have knowledge of what it implements, but I'm not entirely sure what you mean. Because currently it does have knowledge that the TriggerItem is a section or statement. Which then is what allows the instanceof to be used.

@APickledWalrus
Copy link
Member

You can probably grab the indentation from the Node object itself. As for "not having knowledge", while the ScriptLoader should understand the concept of sections, statements, etc. (i.e. the syntax types), it should not have knowledge of (or reference) specific implementations of those types (e.g. conditional sections).

@TheAbsolutionism
Copy link
Contributor Author

TheAbsolutionism commented Jan 14, 2025

You can probably grab the indentation from the Node object itself. As for "not having knowledge", while the ScriptLoader should understand the concept of sections, statements, etc. (i.e. the syntax types), it should not have knowledge of (or reference) specific implementations of those types (e.g. conditional sections).

Alrighty, well I'll delete the MultilinedConditional interface and the instanceof + following statements within ScriptLoader
Should/Can I add a static + instance public #multilinedToString or #toMultilinedString within Conditional allowing a set method to be used to retrieve a string spanning over the appropriate amount of lines? So it can be used by addons and if/when my While All/Any?

@TheAbsolutionism
Copy link
Contributor Author

@APickledWalrus Since the discussion on discord came to an end without a final verdict. Let me know what approach I should do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants