SkriptLang
ExprLore - Cleanup and Changes
Description
This PR aims to ~rewrite~ cleanup the ExprLore class as well as make a test file explicitly for itself.
In addition to all the amazing changes below, I've added early support for 1.21.5's new lore cap, so long as their server is running 1.21.5 the lore will cap itself out at 256
Breaking changes
Yes, I know the team hates them (I don't), but the behaviors were just wrong, and I can't see a reason to deprecate it if nothing else in skript behaves like this. Please read the information below, before reviewing this PR, I don't want to cause too many repeated answers that is already answered below.
The changes here were done to help make skript better and more flexible, I'm well aware of skript's love to lock event-item from modifications, but this should change if the ability isn't overly harmful to the player's experience.
Click for information about changer breaking changes
add, remove and remove all changers have been fully reworked to work only on a list bases than a single string line bases.
ADD
In the past add "test" to line 1 of player's tool's lore internally was just doing line 1 of lore + "test" this made zero sense since add is not something you would expect for concatenation, now that skript has both String + String and concat(String...) additionally join x with y I believe it's best to fully strip out this behavior in favor of only accepting add in add x to lore of y
REMOVE/REMOVE_ALL
Similar to ADD the REMOVE and REMOVE_ALL also had wonky behavior when used along side both lore of x and line x of lore of y
The remove method previously would convert a list like "My", "String", "Coolio" into "My\nString\nCoolio" and use .replace() and/or .replaceAll(), this lead to lore not actually being removed but instead just becoming blank i.e.
remove "String" from lore of {_item} -> "My\n\nCoolio" -> "My", "", "Coolio" this has been changed to now act upon list removal making it explicitly only when doing lore of x and is no longer an accepted change mode for line x of lore of y
I've fully stripped out the creation of blanks in lore besides when using set line x of lore of y to "blank", outside of it's current lore size, I believe this behavior was well received and made sense on a user end of things. However remove will no longer create a blank line when used, meaning someone doing
loop 5 times:
remove "x" from line loop-counter of lore of {_item}
Would instead need to be
loop 5 times:
replace "x" with "" in line loop-counter of lore of {_item}
Click for information about the less strict ChangerUtils#acceptsChange
As someone will eventually point out the internal code no longer runs ChangerUtils.acceptsChange(item, ChangeMode.SET, ItemStack.class, ItemType.class) any time someone does a change, this is now exclusive to when item.getTime() is not EventValues.TIME_NOW. this allows for people to now do set lore of event-item to "hahahha" which was previously unsupported.
From the testing I've done there was no reason for this to be done for TIME_NOW, these *should* be the existing items meaning changes made to them should still apply to 90% of event-items in skript.
People doing set lore of past event-item to ... and set lore of future event-item to ... will still be checked against the ChangerUtils.acceptsChange and correctly return when the new event-value changers are active. Below is a complete list of events I tested this in to showcase the utility in it as well as how much more it opens to the user base. While it is regrettable that events which don't support it will not error, there are far fewer of those than events that it works in.
The events tested in these are events that had event-itemstack and event-itemtype on the Skripthub site, so new 2.11 events were not tested.
Tested events that worked
https://docs.skriptlang.org/events.html?search=#anvil_prepare https://docs.skriptlang.org/events.html?search=#book_edit https://docs.skriptlang.org/events.html?search=#book_sign https://docs.skriptlang.org/events.html?search=#click https://docs.skriptlang.org/events.html?search=#drop https://docs.skriptlang.org/events.html?search=#elytra_boost https://docs.skriptlang.org/events.html?search=#prepare_craft https://docs.skriptlang.org/events.html?search=#enchant_prepare https://docs.skriptlang.org/events.html?search=#enchant https://docs.skriptlang.org/events.html?search=#riptide https://docs.skriptlang.org/events.html?search=#stop_using_item https://docs.skriptlang.org/events.html?search=#player_pickup_arrow https://docs.skriptlang.org/events.html?search=#inventory_pickup https://docs.skriptlang.org/events.html?search=#inventory_item_move https://docs.skriptlang.org/events.html?search=#inventory_click https://docs.skriptlang.org/events.html?search=#item_mend https://docs.skriptlang.org/events.html?search=#item_damage https://docs.skriptlang.org/events.html?search=#item_spawn https://docs.skriptlang.org/events.html?search=#pick_up https://docs.skriptlang.org/events.html?search=#spawn
Tested events that didn't work
https://docs.skriptlang.org/events.html?search=#craft https://docs.skriptlang.org/events.html?search=#armor_change https://docs.skriptlang.org/events.html?search=#dispense https://docs.skriptlang.org/events.html?search=#entity_breed https://docs.skriptlang.org/events.html?search=#item_merge
Niche Special cases
https://docs.skriptlang.org/events.html?search=#place - This works but causes items to become well infinite https://docs.skriptlang.org/events.html?search=#inventory_drag - This works but I am labeling special as it's only for cursor slot when using event-item
NOTE: I am writing this with zero sleep, and if things aren't making sense please let me know so I can update the message.
Target Minecraft Versions: any Requirements: none Related Issues: #6245
Example of old behavior can be seen as
Example of new behavior can be seen as
while i agree with the changes made, i think this is probably too much of a breaking change to be worth it with no syntax change, since items are used everywhere in skript. i'd be okay with locking the new behaviour behind an extra syntax, so the new behaviour would be applied with
remove all "s" from >all< lore of itemfor example.there is precedent for locking better behaviour behind syntax, e.g. #6930. wdyt?
Efy, Efy, our non responsive Efy, I truly hope I did not read you saying remove acting like replace was "better behaviour", remove is by no means replace. This is truly disappointing some of the most disappointment I've felt since the day of my birth.
Your suggestion for adding all is not enough for the little change it offered, it wouldn't fix the breaking issue which I have zero intention of doing so. all is used in places where list are expected, so all lore of item is still just lore of item, example being all items -> items, all players -> players, all %classinfo with supplier%s -> %classinfo with supplier%s (all entity types -> entity types)
The little effort people need to do for supporting replacing from remove is overly easy and simple. As I've said users only need to change remove "blank" from lore of player's tool -> replace "blank" in lore of player's tool with ""
Old behavior broke current behavior conventions, nothing else in skript new or old (not confirmed) did behavior like this. Now I can acknowledge that while remove is easily replaced, add might be a bit more effort on the end user's end.
add "test" to line 1 of player's lore -> set line 1 of item's lore to line 1 of item's lore + "test", however I believe there's future improvement that could be added in place of it. Such as ChangeMode.APPEND, append %object% [on[ ]]to %~object% for append "test" on to line 1 of player's tool
I'm confident that most people did not know you can add onto a specific line of lore since even I didn't until I made this PR so for 7-8 years I've never known this. I've also never seen it used in help channels, but that could also be me entirely missing them.
I won't say it's amazing to have breaking changes, but there's a time when they are needed, I believe this is one of those times where the team/people shouldn't be dragging their feet in making a decision, #7712 is a time where taking a step back and deciding is fair and likely a good call. Lore of an item is a list, it's not a single lined string where replace should be used. Skript needs to follow a convention in handling changers and a list is not a single entry it's multiple. using remove "hello" from lore of player's tool should never make Hello! -> !, it should fail
As a further point, when skript eventually drops spigot and swaps to paper, the team will likely also have to convert to components later down the line. Converting components to string and back into a component will cause data loss depending on how they were made. You the team will likely have to make the decision I'm doing here eventually I believe now is better than never so when you eventually go through with this you don't have to rewrite the whole class..
In my opinion, this seems like something that should start as a discussion. I think we are getting ahead of ourselves by making a PR right now.
In my opinion, this seems like something that should start as a discussion. I think we are getting ahead of ourselves by making a PR right now.
I believe the PR being made, regardless of it's status of being accepted or not, is beneficial. It helps all to know what changes that should be considered and be a point of reference.
Your suggestion for adding all is not enough for the little change it offered, it wouldn't fix the breaking issue which I have zero intention of doing so. all is used in places where list are expected, so all lore of item is still just lore of item, example being all items -> items, all players -> players, all %classinfo with supplier%s -> %classinfo with supplier%s (all entity types -> entity types)
the exact syntax does not matter, as long as you are able to differentiate it from the old way of removing.
The little effort people need to do for supporting replacing from remove is overly easy and simple. As I've said users only need to change remove "blank" from lore of player's tool -> replace "blank" in lore of player's tool with ""
this is still a change users need to do. they may need to replace hundreds of these occurrences across years of scripts, which is why a non-breaking change is, in my opinion, the only acceptable solution here.
add "test" to line 1 of player's lore -> set line 1 of item's lore to line 1 of item's lore + "test"
the second option here is literally exactly what the point of using add is: to remove having to reference the same expression twice. why is the second option better?
As a further point, when skript eventually drops spigot and swaps to paper, the team will likely also have to convert to components later down the line. Converting components to string and back into a component will cause data loss depending on how they were made. You the team will likely have to make the decision I'm doing here eventually I believe now is better than never so when you eventually go through with this you don't have to rewrite the whole class.
i highly doubt components are going to be required as soon as they are implemented. we can't just break every chat string and item string. also, if we're trying to future-proof, why would we accept this pr if we're going to make it use components later on anyway?
Efy, Efy, our non responsive Efy, I truly hope I did not read you saying remove acting like replace was "better behaviour", remove is by no means replace. This is truly disappointing some of the most disappointment I've felt since the day of my birth.
finally, please refrain from rhetoric like this. how am i supposed to enjoy reviewing your prs if this is the level of respect i get for spending my voluntary free time on skript? if you do this again, i will not be reviewing your prs anymore. we should all respect each other as volunteers.
the exact syntax does not matter, as long as you are able to differentiate it from the old way of removing.
the second option here is literally exactly what the point of using
addis: to remove having to reference the same expression twice. why is the second option better?
If we can form an agreement on still removing the remove support add is still acknowledged as useful, but remove has no place in this expression. The old way of removing is poor behavior and unlike add is easily replaced with a proper method, without it causing poor behavior like empty lore lines or replacing World in World!.
If you're unable to openly accept this change, then I don't have any other argument to make with you as you've made-up your mind, and changing a person's mindset is not easily achieved (Exhibit-A: pikachu).
Alternatives should still be looked into for skript's end in-place of add since it's still poor behavior from my perspective.
this is still a change users need to do. they may need to replace hundreds of these occurrences across years of scripts, which is why a non-breaking change is, in my opinion, the only acceptable solution here.
I believe that's the point of breaking change, to force a user to change behavior. Retaining previous behavior is not good behavior, I don't see why you want it retained just because it's breaking. Changes are happening regardless of whether anyone wants them, Skript needs to move forward with things and stop holding onto the past, this has been a continuous debate with the team because of their negligence. This has been seen many times with most recent being complaints being ItemType.
What you call an "acceptable solution" is not acceptable in my opinion, remove had no documentation on how it worked within the description, and anyone who used it would expect it to work as it does with remove "Hello" from {_myList::*} (i.e. it wouldn't remove from "Hello!" but it would remove "Hello")
i highly doubt components are going to be required as soon as they are implemented. we can't just break every chat string and item string. also, if we're trying to future-proof, why would we accept this pr if we're going to make it use components later on anyway?
Correct!!! 🎉🎉🎉🎉
They won't be forced right away, well more accurately you can't, after all skript would still have to go through how to handle everything since adventure components are not mutable, every change returns a whole new component. So a basic implementation within skript can't be done, and would require many changes for them to be functional, however we also have to acknowledge paper has deprecated String methods for years and with Skript still supporting spigot, no one outside of the team knows when you plan to make a final spigot supported release. will it be before or after paper removes spigot's methods? With the direction paper is going, I hope to see this decision by the end of the year, since we're already seeing paper exclusive content when paper 1.21.5 is released. (i.e. new variants for farm animals)
As for not accepting this pr, if you're just going to update it again in the future, what kind of reasoning is that? Sure you might have to rewrite, but if the change will occur either way, giving your users the ability to update now is better than months or years down the line.
finally, please refrain from rhetoric like this. how am i supposed to enjoy reviewing your prs if this is the level of respect i get for spending my voluntary free time on skript? if you do this again, i will not be reviewing your prs anymore. we should all respect each other as volunteers.
I would apologize, but in truth that's just lying, being rhetoric is my way of speaking, and if you can't accept that there's not much I can do, being a volunteer or a member won't change the way I speak to you or anyone else.
If you don't wish to review my PRs I'm perfectly fine with that, as it's volunteer work for you, all I ask is instead of ignoring those prs as you are a member of the team remove your code review request and put someone else in place so the prs don't sit there idle like many old prs have. I would still like to keep the process moving forward even if you don't wish to be involved.
And @Burbulinis, if you're going to 👍 it's strongly encouraged to use your big boy words, this is a debate, not a poll with a yeah or no answer
If anyone else has anything they want to say feel free, for now I will see you all in 3 days when the next reply occurs
So...we mark it as "abuse", the only none structured argument within that whole message was calling out the team for their lack of communication. Which mind you was proved by your lack of words, and out of spite using reactions.
If someone would care to explain how that's labeled abuse, would love it otherwise, that's just disrespectful to me as well. I've given you the option of adding back add which could be seen as valid behavior, where as remove is not.