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

Script development auto reloading on save #7464

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

Conversation

TheLimeGlass
Copy link
Contributor

@TheLimeGlass TheLimeGlass commented Jan 17, 2025

Description

  • Adds structure to denote automatic script reloading per script.
  • Adds entryValidator method to set an EntryValidator for a structure.
auto reload:
        # Cancels out permission node. Define the players that will get reload notifications.
	recipients: "LimeGlass" and "Dinnerbone"

	# Players with this permission will receive reload notifications.
	permission: "skript.reloadnotify"

Target Minecraft Versions: any
Requirements: none
Related Issues: #4313

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.

The entry validator is a parameter for registering the structure. Is there a reason you aren't/can't just do that?

@TheLimeGlass
Copy link
Contributor Author

The entry validator is a parameter for registering the structure. Is there a reason you aren't/can't just do that?

Oh no. Didn't see that. Interesting way of setting the EntryValidator. I still believe this overriding implementation is good so it can be changed dynamically.

@APickledWalrus
Copy link
Member

If it needs to be done dynamically (which seems unlikely), I think it would be better for the developer to manage it in init themselves

@TheLimeGlass
Copy link
Contributor Author

If it needs to be done dynamically (which seems unlikely), I think it would be better for the developer to manage it in init themselves

You'd have to set the EntryValidator by getting the StructureData then the StructureInfo, and that would override the existing EntryValidator if it was also needed because the Structure init is called before the implemented Structure.

@APickledWalrus
Copy link
Member

They can simply store the EntryValidator on the class and validate it (receive an entry container) themselves in the overridden init. It does't necessarily have to be the internal one. This is the common approach for addon developers who implement these for Sections.

@Pikachu920
Copy link
Member

I'd rather see this as an addon or script personally

@TheLimeGlass
Copy link
Contributor Author

TheLimeGlass commented Jan 17, 2025

They can simply store the EntryValidator on the class and validate it (receive an entry container) themselves in the overridden init. It does't necessarily have to be the internal one. This is the common approach for addon developers who implement these for Sections.

Ya that's one way of doing it. It probably should've been an overriding method from the start to avoid storing it in ram, like Njol did with everything. Changers for example.

Copy link
Contributor

@ShaneBeee ShaneBeee left a comment

Choose a reason for hiding this comment

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

I was interested in trying this, but ran into a few minor issues.

@Moderocky Moderocky added the up for debate When the decision is yet to be debated on the issue in question label Jan 17, 2025
@APickledWalrus
Copy link
Member

They can simply store the EntryValidator on the class and validate it (receive an entry container) themselves in the overridden init. It does't necessarily have to be the internal one. This is the common approach for addon developers who implement these for Sections.

Ya that's one way of doing it. It probably should've been an overriding method from the start to avoid storing it in ram, like Njol did with everything. Changers for example.

We don't want to be creating an EntryValidator every time the Structure is parsed. I don't think storing it in RAM is anything to be concerned about (or how much memory that might be - it won't be that much). Handling the validation through the syntax info is fine as-is.

@TheLimeGlass
Copy link
Contributor Author

TheLimeGlass commented Jan 17, 2025

We don't want to be creating an EntryValidator every time the Structure is parsed. I don't think storing it in RAM is anything to be concerned about (or how much memory that might be - it won't be that much). Handling the validation through the syntax info is fine as-is.

It'll be needed at parse time since that's when the Structure is created and parsed. Once the EntryValidator is used during parsing, it'll be garbage collected.

@TheLimeGlass TheLimeGlass marked this pull request as draft January 17, 2025 23:27
@TheLimeGlass
Copy link
Contributor Author

Placing in draft, as the parsing doesn't support String lists.

@TheLimeGlass TheLimeGlass marked this pull request as ready for review January 18, 2025 00:08
@Dummybell
Copy link

add it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
up for debate When the decision is yet to be debated on the issue in question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants