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

DefaultOperations.java - Add blank string default #7712

Draft
wants to merge 4 commits into
base: dev/feature
Choose a base branch
from

Conversation

Fusezion
Copy link
Contributor

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

@Pikachu920
Copy link
Member

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 Pikachu920 added enhancement Feature request, an issue about something that could be improved, or a PR improving something. up for debate When the decision is yet to be debated on the issue in question labels Mar 16, 2025
@Absolutionism
Copy link
Contributor

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.

@ShaneBeee
Copy link
Contributor

ShaneBeee commented Mar 16, 2025

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

@sovdeeth
Copy link
Member

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.

@ShaneBeee
Copy link
Contributor

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.

@Absolutionism
Copy link
Contributor

I agree with Shane

@sovdeeth
Copy link
Member

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: #6576

@ShaneBeee
Copy link
Contributor

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

@sovdeeth
Copy link
Member

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

@ShaneBeee
Copy link
Contributor

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

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

@sovdeeth
Copy link
Member

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.

@ShaneBeee
Copy link
Contributor

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.

@Pikachu920
Copy link
Member

Pikachu920 commented Mar 16, 2025

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++.

@ShaneBeee
Copy link
Contributor

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.

@sovdeeth
Copy link
Member

@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.

@Fusezion
Copy link
Contributor Author

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.

@sovdeeth
Copy link
Member

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.

@Fusezion
Copy link
Contributor Author

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"

@sovdeeth
Copy link
Member

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.

@Absolutionism
Copy link
Contributor

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

@sovdeeth
Copy link
Member

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

There's no consensus yet, so no.

@Pikachu920
Copy link
Member

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
Copy link
Member

Pikachu920 commented Mar 18, 2025

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.

@sovdeeth
Copy link
Member

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.

@Pikachu920
Copy link
Member

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?

@sovdeeth
Copy link
Member

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.

@Pikachu920
Copy link
Member

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

@sovdeeth
Copy link
Member

sovdeeth commented Mar 18, 2025

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.

@Pikachu920
Copy link
Member

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

@Absolutionism
Copy link
Contributor

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.

@Pikachu920
Copy link
Member

Pikachu920 commented Mar 18, 2025

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

I agree!

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?

Yeah, I just mean that there's no need to go super quickly. We can take a moment to let the idea set and revisit in a few months, no big deal. I've had plenty of things I've added in the past (especially early on) that I think could have really benefited from stepping back and intentionally take some time.

On another note, your replies can often make me feel as though I am being perceived as an "enemy" in some fashion. I would really appreciate if you could give me the benefit of the doubt. I really have a limited energy for contributing to Skript these days, and I promise I only still do it at this point because I care about the language and the community.

It can be really frustrating when I spend some of this limited energy on trying my best to engage politely and fairly, and then someone replies in a way that feels as if my words are being received in a manner that assumes I have some ulterior motives. We are all just volunteers here, and I know you care about the language too. Please, let's work together on that common goal 🙂❤️.

@ShaneBeee
Copy link
Contributor

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

My fear with sitting on this too long is, it skips a "major" update, skripters get used to this odd behaviour and 1 of 2 things happens:

  1. A "breaking" change is made down the road after people are used to one way, and often get upset.
    (I honestly don't think anyone would be upset in this scenario since the change would benefit the community)

  2. No change is made because "people are used to this by now"

I am by no means pushing for this to be done tomorrow, but it would be nice if a consensus could be decided upon before the 2.11 reason on (or around) April 1st.

It seems like the consensus to allow string + object#toString() has been reached as a positive majority, and something that would benefit the community. That part could make its way into Skript for 2.11, and I don't think anyone would be against this behaviour (at least the active users of the plugin).

@Absolutionism
Copy link
Contributor

Yeah, I just mean that there's no need to go super quickly.

It's not really about that this needs to be decided ASAP. It's that things labeled as up for debate should have debates and there is one going on for this right now, which is a step forward. It would be more beneficial to not only the PR author but also the team for this debate to continue rather than being held off for an unreasonable amount of time. Even if the debate was to take a couple more days or even a month, atleast it would be debated and not sit here. Calling off a debate for what seems to be no reason at all, just makes no sense to me.

my words are being received in a manner that assumes I have some ulterior motives.

I know that you don't have ulterior motives. The one thing I do know is that when you make up your mind, you don't change it. Which can be proven through the multiple suggestions that have been created since January where you have disagreed and labeled as up for debate.

I am being perceived as an "enemy"

The only "enemy" I would consider you to be is against quality of life and useful additions towards the users, that actually use Skript, that could benefit from said additions.

@sovdeeth
Copy link
Member

sovdeeth commented Mar 18, 2025

That part could make its way into Skript for 2.11, and I don't think anyone would be against this behaviour (at least the active users of the plugin).

I am not sure something like this is necessarily going to make it into 2.11 even if we start implementing it today, just with the amount of time left. I could be wrong, but the changes made to ExprArithmetic I did to make a prototype were not super trivial and I'd want a good set of tests and at least a review by tud before being confident with it. It definitely could make it, but I don't think we should rush discussion just for the sake of 2.11. I don't think we need to take a month or whatever but I would like the input of more team members, especially at least another core maintainer to chime in.

@Pikachu920
Copy link
Member

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

My fear with sitting on this too long is, it skips a "major" update, skripters get used to this odd behaviour and 1 of 2 things happens:

1. A "breaking" change is made down the road after people are used to one way, and often get upset.
   (I honestly don't think anyone would be upset in this scenario since the change would benefit the community)

2. No change is made because "people are used to this by now"

I am by no means pushing for this to be done tomorrow, but it would be nice if a consensus could be decided upon before the 2.11 reason on (or around) April 1st.

It seems like the consensus to allow string + object#toString() has been reached as a positive majority, and something that would benefit the community. That part could make its way into Skript for 2.11, and I don't think anyone would be against this behaviour (at least the active users of the plugin).

Yeah, we can certainly continue to talk about it however I do feel aiming to get this in for 2.11 is a jumping the gun a bit. Skript is pretty unusual in how it handles changes like this because there is no formal proposal process like you find in other languages. Above all, I think the most important thing we can try to deliver to Skript users is stability and predictability. I think it's best for Skript's long term health if we pause for a moment before changing fairly fundamental aspects of the language (like the behavior of an operator). The conversation doesn't have to stop in the meantime! This PR has only been open for two days - in my eyes, it would be great to allow others the opportunity to chime in with their perspective or to point out things we may have missed.

Since I don't think we should get it in for 2.11, I'd love to target the end of April instead. If the consensus at that point is that we want it, that's totally fine with me. That's just about a month and a half, which is pretty much lightspeed as far as language proposals are concerned.

@ShaneBeee
Copy link
Contributor

That part could make its way into Skript for 2.11, and I don't think anyone would be against this behaviour (at least the active users of the plugin).

I am not sure something like this is necessarily going to make it into 2.11 even if we start implementing it today, just with the amount of time left. I could be wrong, but the changes made to ExprArithmetic I did to make a prototype were not super trivial and I'd want a good set of tests and at least a review by tud before being confident with it. It definitely could make it, but I don't think we should rush discussion just for the sake of 2.11. I don't think we need to take a month or whatever but I would like the input of more team members, especially at least another core maintainer to chime in.

Thank you for this reply.
This is the kind of logical reply we need.

@sovdeeth
Copy link
Member

This PR has only been open for two days - in my eyes, it would be great to allow others the opportunity to chime in with their perspective or to point out things we may have missed.

Since I don't think we should get it in for 2.11, I'd love to target the end of April instead.

I'm not sure we need to put any specific date on this, since I fear it's rare for discussions to start back up after people have said their piece, but I do agree that it's been open for a very short time and there is a need for more voices before we make any decisions. I suppose having the end of april is at least some sort of deadline but I don't think we need to necessarily wait until then if we get some more thoughts in the next week or two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. 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.

5 participants