TriggerReactor

TriggerReactor

24.6k Downloads

Add deprecation message for $ sign on script loading

gerzytet opened this issue · 11 comments

commented

Code such as:
#MESSAGE "you need $3 to buy a sponge"
will work in 2.1.8, but not 2.2, thanks to the new interpolation features added by c11f67e.
To avoid breaking such code, placeholder interpolation needs to be explicitly specified by requiring use of the , or by declaring the string in some special way (such as python's raw strings, or "f-strings")
I believe the former is easier to do.

commented

I think we can address this issue with the config file

Maybe add an option like useAutomaticPlaceholderConversion=true

so that the new comers (usually who don't understand String concatenation) will benefit from the automatic conversion, yet the previous code flow can be compatible if set the option to false

This will require #167 to be done first though.

commented

It needs to be false be default then

commented

I still believe that changing the syntax is the best way to go. The current syntax defines an entirely new special-meaning string character other than \ (unusual in most languages).
If it was a global config, it would introduce further inconsistency to the language. We would have to start telling people to change their configuration options just to be able to use the new feature, but changing it may introduce the incompatibility issues with other triggers.
If it was local to triggers, we would have to worry about adding that config option to every single trigger, including named triggers, who don't have one.
If the goal is to release the feature with backwards compatibility, consistency, in a way that is simple to implement and understand, changing the syntax is the only way to go.

commented

If you don't like the look of "\$playername is in \$worldname"
consider this: $"$playername is in $worldname"

commented

Wouldn't that disqualify the justification of this commit entirely?

The reason for this commit is that often, new comers put placeholders inside the string and presumably think it will work, just like how VT works.

commented

Sadly, it's indistinguishable whether the $ sign is a placeholder or not.

For example, $3 can be a placeholder named 3, but it's impossible to know the differences.

Though c11f67e also introduces the escape sequence, so one who wants to fix the problem can add escape sequence instead.

#MESSAGE "you need \$3 to buy a sponge"

commented

That fixes the problem for anyone writing new code, but still requires server owners track down and apply those fixes when they switch to the new version.
In other words, it's incompatible.

commented

If you want to implement the syntax the way it currently is, you need to be very careful that everyone knows what’s coming, and that you give people time to adjust, meaning that the feature probably isn’t right for 2.2
That would begin by making an unescaped $ in strings a warning or an error.
It’s just not fair to suddenly implement a feature that could break existing code without telling everyone

commented

Will add deprecation message to the 2.2 release version.

Automatic placeholder conversion will be added to 3.0.0

commented

sounds good to me
I think the best time to display these messages would be on plugin load/reload

commented

Implemented in release 2.2.0