Skript icon indicating copy to clipboard operation
Skript copied to clipboard

DefaultOperations.java - Add blank string default

Open Fusezion opened this issue 8 months ago • 38 comments

Description

This PR aims to add a new operations default of "" when someone does "example" + {_unset} The issue this is related to is marked as "Up for Debate", however from my perspective I see no harm in skript handling null with blank. I will mark this PR as draft, until a decision is firmly made by the team, any other feedback is welcome

While it's true in java "blank" + null throws a null point exception, there's also work arounds like Objects.toString() and String.valueOf(), I think there's more benefit from this over none.

If the main concern is someone does {_blank} + "Hello" and doesn't correctly handle their variable, then I believe having the ability to actually see the string missing their inputted type makes more sense than it abruptly becoming null especially in scenarios where they're doing "This " + {_that} + " and this" an error in their code is clearly visible and debuggable.


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

Fusezion avatar Mar 16 '25 11:03 Fusezion

I personally prefer defaulting to null instead of an empty string. If users want an an empty string as a default, they can explicitly specify ("This" + ({_that} otherwise "") + "and this")

Pikachu920 avatar Mar 16 '25 14:03 Pikachu920

I believe this proposal aligns well with Skript’s goal of being user friendly, especially for those without programming experience. The current behavior of string concatenation seems like an oversight, as when stringifying a null variable it would output "<none>", but yet concatenation causes the string to be voided. This difference between the two is inconsistent which presents itself as a bug. Since logically it makes no sense. Requiring users to manually handle null values with {_} otherwise "" is counterintuitive. A single oversight in how a user handles null values or an error to occur within an automated process can break the script. By defaulting it to "" it can at the very least return a string, even if the result is not as expected. Additionally, it can even be used as a feature, allowing users to use the defaulted value to their advantage. If defaulting to "" is still a concern or is not satisfactory, an alternative could be defaulting it to "<none>" which would match the current behavior when stringifying.

Absolutionism avatar Mar 16 '25 20:03 Absolutionism

Throwing my 2 cents in:

  1. I dont like the idea of an empty string, I think it should be the same as stringifying it, it should send "<none>"

  2. I noticed it does the same with non-string objects. This feels bad. Screenshot 2025-03-16 at 2 02 34 PM

Screenshot 2025-03-16 at 2 02 51 PM

ShaneBeee avatar Mar 16 '25 21:03 ShaneBeee

I think this is a good spot for runtime errors to be used. I don't think adding unset values should work in general, but the behavior of null = 0 in math has been grandfathered in, sadly. I don't think that's worth changing at this point.

I think that any invalid operation (string + number, string + null, null + timespan, ...) should return null and produce a runtime error. It's the most explicit and clear way to handle ambiguous or nonsensical math.

sovdeeth avatar Mar 16 '25 21:03 sovdeeth

I feel like Skript should match its parent language... java. In java if you do "blah" + unSetVar it'll send Null. If you do "blah" + anyOtherObject it'll run its #toString()

I haven't really done much in other languages, but I'd assume they're similar.

ShaneBeee avatar Mar 16 '25 21:03 ShaneBeee

I agree with Shane

Absolutionism avatar Mar 16 '25 21:03 Absolutionism

I feel like Skript should match its parent language... java. In java if you do "blah" + unSetVar it'll send Null. If you do "blah" + anyOtherObject it'll run its #toString()

I haven't really done much in other languages, but I'd assume they're similar.

I think this would be fine if skript were a typed language, but given the nature of variables being any type, this could result in unintentional string concatenation. I think enforcing "%%" is a better idea to make it clear you want it as a string, similar to python's str() function. There's some more explanation in the original string concat pr too: https://github.com/SkriptLang/Skript/pull/6576

sovdeeth avatar Mar 16 '25 21:03 sovdeeth

In my opinion, having a mismatch between "%%" v. concat seems a little confusing and super inconsistent.

ShaneBeee avatar Mar 16 '25 21:03 ShaneBeee

Can you elaborate on what you mean by mismatch? I'm not sure what you're referring to

sovdeeth avatar Mar 16 '25 21:03 sovdeeth

send "Blah: %{empty}%" -> blah: <none> send "Blah:" + {empty} -> well... nothing prints

send "Blah: %{worldVar}%" -> Blah: world send "Blah:" + {worldVar} -> also prints nothing

ShaneBeee avatar Mar 16 '25 21:03 ShaneBeee

send "Blah: %{empty}%" -> blah: <none> send "Blah:" + {empty} -> well... nothing prints

send "Blah: %{worldVar}%" -> Blah: world send "Blah:" + {worldVar} -> also prints nothing

Gotcha. I think there's two ways to approach this currently: a) hardline string concatenation (this is what Python does) This is Skript's current stance. "a" + "b" is simply for concatenating strings, no more, no less. The current behavior is self-contained and makes sense in this view. Adding a world shouldn't work, since it's not a string. Adding null shouldn't work, it's not a string. Using "%%" is not concatenation, it's insertion and coercion, and therefore runs on different rules. I think if you're happy with this definition, runtime errors are the best answer. b) coercive string concatenation (this is what Java does) I believe this is what you're proposing. "a" + "b" is for building strings in general, and should have features that makes building them easier. In this view, adding worlds should work like how insertion does, by coercing it into a string first. In this view, I agree that a null value should be interpreted as "", as it's the string representation of an unset value in skript. If you want this definition, the behavior should match insertion as closely as possible.

I personally think a) is the safest approach for skript, but I have certainly found it a little unintuitive, especially after using java (I feel the same with python, so this isn't necessarily saying java's way is the best, only that skript's is different from java's). I would be ok with b) if it's able to be implemented safely without affecting any other types of math (I have no clue if this is the case or not), though.

sovdeeth avatar Mar 16 '25 21:03 sovdeeth

Adding a world shouldn't work, since it's not a string.

I don't see why it couldn't. If Java can do it, other than SkriptLang team saying no... what's stoping this behaviour? For consistencies sake, it just makes sense (at least to me it does)

Using "%%" is not concatenation, it's insertion and coercion, and therefore runs on different rules.

If we look at Java's String.format("blah %s", emptyVar) ... it acts the same way as Skript, will produce "blah null" So it would make sense if string concat also matched the behaviour of Java. Using Java's new (preview) String templates does the same thing. I don't know how Python works (never used it), so I'm just going off of what I know of java.

I think if you're happy with this definition, runtime errors are the best answer.

You saw how well that worked when I added them in SkBee for empty NBT 😂 .... people are going to hit the fan when they get runtime errors.

b) Same as mentioned above.

Just wanted to finish off with again... I think there (like Java) should be consistency between insertion and concat.

ShaneBeee avatar Mar 16 '25 21:03 ShaneBeee

I feel like Skript should match its parent language... java. In java if you do "blah" + unSetVar it'll send Null. If you do "blah" + anyOtherObject it'll run its #toString()

I wouldn't really call Java the parent language of Skript. it's just what skript is written in, for better or worse. This is not too different from saying "I feel like Java should match it's parent language... C++" just because most JVMs are written in C++.

Pikachu920 avatar Mar 16 '25 21:03 Pikachu920

I feel like Skript should match its parent language... java. In java if you do "blah" + unSetVar it'll send Null. If you do "blah" + anyOtherObject it'll run its #toString()

I wouldn't really call Java the parent language of Skript. it's just what skript is written in, for better or worse. This is not too different from saying "I feel like Java should match it's parent language... C++" just because most JVMs are written in C++.

My statement (maybe worded incorrectly) is referencing the fact that Skript is going to be limited to what Java can/can't do.... but also being the fact it's written in Java, Skript should share some consistencies. If people upgrade from Skript to Java in the future, its nice to have similar behaviours.

ShaneBeee avatar Mar 16 '25 21:03 ShaneBeee

@ShaneBeee I think you misinterpreted the formatting of my message a bit. A) and B) were two design decisions you can choose to follow. Of course, you can coerce a world into a string, that's what java does. But if you are using the design decision that string concatenation should not coerce types, then you cannot concatenate a world with a string.

As far as format, yes, that's string insertion. Python has a similar formatting method, but doesn't allow concatenation of differing types. Similar behaviors vary wildly across langauges, so I don't think we should base choices on what a specific language does or doesn't do, but rather how that choice affects the end result in Skript.

Runtime errors here I think would work fine. The case with SkBee is that they were overused (imo) and often triggered in scenarios that users deliberately intended. In this case, any invalid operation will already result in a failure state and return none, which i think is a great place for an error. I don't envision a situation in which the desired result of any math operation is none, hence I think that an error fits well here.

sovdeeth avatar Mar 16 '25 22:03 sovdeeth

Just going with general talk, I'm fine not doing "" in place of <none> which as people have said makes sense with the fact send "%{_none}%" returns <none>, I can change that but I do not believe sending an error when using "" + {_none} is correct skript should handle that case.

I can agree to sending an error when someone does "" + world of player/any other non string however "" + world of player should have worked since there's a converter for world to string (well now AnyNamed), so something failed to occur there then.

I would be a little confused on how to add the error as I'm still not great at understanding runtime errors due to lack of any documentation for implementation.

Fusezion avatar Mar 16 '25 23:03 Fusezion

I talked with Shane a bit more on the skriptlang discord about this and I think there's 2 routes that could be good here: The conservative route would basically be what Fuse just said, don't change anything about string concatenation but add runtime errors (debatable whether unset strings should have default values or not) The more extensive change would be to add a String + Object operation that converts objects to strings and concatenates them, as well as adding a "" default value and errors for illegal operations. I think this would be relatively safe to add and be intuitive, as it wouldn't run into some of the ambiguity a commutative operator would:

but what about a user trying to convert a string they got from say, a placeholder, into a number set {_value} to 7 + {_my placeholder} would it be better to coerce 7 into a string, or throw an error?

I'm ok with both routes, though I think I would prefer the more extensive changes as I've definitely tried to use + for concatenation and wanted to have it coerce objects into strings automatically.

sovdeeth avatar Mar 17 '25 00:03 sovdeeth

Since my brain is not yet working at full functionality, does this mean we're good with adding a String + Object then?

Since the quoted part mentioned 7 + "string" should we force only String + Object, and not allow Object + String? I had concerns about objects, but so long as only String + Object is supported I believe it's safe enough. If people want Object + String I believe it's fine to force them to use "" + myObject + " my other string"

Fusezion avatar Mar 17 '25 00:03 Fusezion

Since my brain is not yet working at full functionality, does this mean we're good with adding a String + Object then?

Since the quoted part mentioned 7 + "string" should we force only String + Object, and not allow Object + String? I had concerns about objects, but so long as only String + Object is supported I believe it's safe enough. If people want Object + String I believe it's fine to force them to use "" + myObject + " my other string"

Yes string + object only is safe imo. I don't know what others think of that, though.

sovdeeth avatar Mar 17 '25 00:03 sovdeeth

So I presume up for debate is going to be removed correct?

Absolutionism avatar Mar 17 '25 18:03 Absolutionism

So I presume up for debate is going to be removed correct?

There's no consensus yet, so no.

sovdeeth avatar Mar 17 '25 18:03 sovdeeth

Since my brain is not yet working at full functionality, does this mean we're good with adding a String + Object then? Since the quoted part mentioned 7 + "string" should we force only String + Object, and not allow Object + String? I had concerns about objects, but so long as only String + Object is supported I believe it's safe enough. If people want Object + String I believe it's fine to force them to use "" + myObject + " my other string"

Yes string + object only is safe imo. I don't know what others think of that, though.

If someone wants this behavior, I would prefer to see them use join {_mixed strings and objects::*}. Or just a variable string (`"string %{_object}%") in simple cases. I think adding string + object is more likely to cause confusion or frustration in cases like:

command addOneTo <text>:
  trigger:
    send arg + 1 # oops, this sends "11"

I see the desire to have string + object, but I don't think it is worthwhile when we already have perfectly fine and more explicit tools available for this.

Pikachu920 avatar Mar 17 '25 23:03 Pikachu920

Since my brain is not yet working at full functionality, does this mean we're good with adding a String + Object then? Since the quoted part mentioned 7 + "string" should we force only String + Object, and not allow Object + String? I had concerns about objects, but so long as only String + Object is supported I believe it's safe enough. If people want Object + String I believe it's fine to force them to use "" + myObject + " my other string"

Yes string + object only is safe imo. I don't know what others think of that, though.

If someone wants this behavior, I would prefer to see them use join {_mixed strings and objects::*}. Or just a variable string (`"string %{_object}%") in simple cases. I think adding string + object is more likely to cause confusion or frustration in cases like:

command addOneTo <text>:
  trigger:
    send arg + 1 # oops, this sends "11"

I see the desire to have string + object, but I don't think it is worthwhile when we already have perfectly fine and more explicit tools available for this.

actually, I guess ExprJoinSplit doesn't support objects like the broadcast expression. Adding that could be a way forward, but I do have concerns about how that might change how existing code is parsed. Interestingly, it does have concatenate in the pattern for join. I still would rather not see string + object added as I think the additional ambiguity is not worth avoiding using variable strings.

Pikachu920 avatar Mar 18 '25 00:03 Pikachu920

Since my brain is not yet working at full functionality, does this mean we're good with adding a String + Object then? Since the quoted part mentioned 7 + "string" should we force only String + Object, and not allow Object + String? I had concerns about objects, but so long as only String + Object is supported I believe it's safe enough. If people want Object + String I believe it's fine to force them to use "" + myObject + " my other string"

Yes string + object only is safe imo. I don't know what others think of that, though.

If someone wants this behavior, I would prefer to see them use join {_mixed strings and objects::*}. Or just a variable string (`"string %{_object}%") in simple cases. I think adding string + object is more likely to cause confusion or frustration in cases like:

command addOneTo <text>:
  trigger:
    send arg + 1 # oops, this sends "11"

I see the desire to have string + object, but I don't think it is worthwhile when we already have perfectly fine and more explicit tools available for this.

~~I do want to note that join does not work with non-strings currently, so we may want to change that.~~

As for your example, I agree it's unintentional but it's also a pretty easy mistake to find the source of, in my opinion. Currently, that code would simply not send anything and that's harder to figure out and fix. Of course, with runtime errors that gets fixed. My take is that it's worth it to take that small idiosyncrasy in return for a much more flexible string builder.

sovdeeth avatar Mar 18 '25 00:03 sovdeeth

that code would simply not send anything and that's harder to figure out and fix

shouldn't it parse error since the argument return type is known?

Pikachu920 avatar Mar 18 '25 00:03 Pikachu920

that code would simply not send anything and that's harder to figure out and fix

shouldn't it parse error since the argument return type is known?

Oh true, my mistake. Imagine a named argument instead, then.

sovdeeth avatar Mar 18 '25 00:03 sovdeeth

that code would simply not send anything and that's harder to figure out and fix

shouldn't it parse error since the argument return type is known?

Oh true, my mistake. Imagine a named argument instead, then.

Yeah it would be hidden in that case. The last thing I'd point to is that javascript has this behavior, and I think it is pretty widely considered to be a not great thing

Pikachu920 avatar Mar 18 '25 00:03 Pikachu920

that code would simply not send anything and that's harder to figure out and fix

shouldn't it parse error since the argument return type is known?

Oh true, my mistake. Imagine a named argument instead, then.

Yeah it would be hidden in that case. The last thing I'd point to is that javascript has this behavior, and I think it is pretty widely considered to be a not great thing

Yeah that's also my main argument against it. I think we would have to accept that kind of ambiguity in some cases, but we effectively already do with things like timespans and dates also having math operations. It's already unknown what {_a} + {_b} is really doing until runtime. I think that Skript's semi-typing in the form of needing types for function parameters and commands also helps clarify what's going on a bit to the reader, which all adds up to basically a big shrug. I think it's enough to tip the scales towards having the usefulness of string + object over sticking with a slightly less ambiguous addition operator for me.

I also wonder about adding a config-suppressible warning when a string is added to, say, a literal number. It may be worth it to introduce people to the concept if they didn't realise it worked like that at first.

sovdeeth avatar Mar 18 '25 00:03 sovdeeth

that code would simply not send anything and that's harder to figure out and fix

shouldn't it parse error since the argument return type is known?

Oh true, my mistake. Imagine a named argument instead, then.

Yeah it would be hidden in that case. The last thing I'd point to is that javascript has this behavior, and I think it is pretty widely considered to be a not great thing

Yeah that's also my main argument against it. I think we would have to accept that kind of ambiguity in some cases, but we effectively already do with things like timespans and dates also having math operations. It's already unknown what {_a} + {_b} is really doing until runtime. I think that Skript's semi-typing in the form of needing types for function parameters and commands also helps clarify what's going on a bit to the reader, which all adds up to basically a big shrug. I think it's enough to tip the scales towards having the usefulness of string + object over sticking with a slightly less ambiguous addition operator for me.

I also wonder about adding a config-suppressible warning when a string is added to, say, a literal number. It may be worth it to introduce people to the concept if they didn't realise it worked like that at first.

perhaps we can sit on it for a month or two and see how it feels then? I don't think this is needed so urgently that we can't take a moment to be intentional about our decision

Pikachu920 avatar Mar 18 '25 00:03 Pikachu920

I don't think this is needed so urgently

Technically speaking, anything new added into Skript isn't urgent unless to fix a major bug.

perhaps we can sit on it for a month or two and see how it feels then?

Can you explain what you mean by this? You want the current debate to just end and pick back up in a month or two? Regardless of your intention by this, seems like poor behavior. Why hold off the debate that is happening? Especially when we all know when the 1 or 2 month time is up, you will still dislike this idea.

Absolutionism avatar Mar 18 '25 00:03 Absolutionism