SkriptLang / Skript

Skript is a Spigot plugin which allows server admins to customize their server easily, but without the hassle of programming a plugin or asking/paying someone to program a plugin for them.
https://docs.skriptlang.org
GNU General Public License v3.0
1.06k stars 368 forks source link

Add config option to toggle the #1149 warning message. #1278

Closed GiraffeCubed closed 6 years ago

GiraffeCubed commented 6 years ago

With dev35 came the #1149 "%text% is already a text, so you should not put it on one" warning, but unlike other warning messages, there's no config option to disable this.

While some may advise against using variables in a text like this, it can be useful in some situations. If we feel the warnings are not necessary for us, we should be able to disable them.

Snow-Pyon commented 6 years ago

I already said my opinion about this. I won't add it for now.

GiraffeCubed commented 6 years ago

I hope it comes eventually, there are situations where it's preferable to write things this way, it'll be nice not being nagged about it for ever.

Also not sure where your opinion on this was, how come it won't be added for now?

bensku commented 6 years ago

I would be interested to hear in what cases it is preferred to create a string of something that is already a string. With current Skript versions, there should be none.

TheBentoBox commented 6 years ago

Why should we let users remove a warning which prevents them from performing unnecessary operations? If you see that error, it means you do not need to be forcibly string casting and it's for the better of your code that you fix it. And it's not like string casting even looks really nice in Skript.

I get that it's annoying that this was added so far into Skript's development as to add errors to a bunch of old scripts, but it's for the better. People are often confused about where and why to add their "%%"'s and this helps add clarity while improving code quality.

Pikachu920 commented 6 years ago

My opinion on this is that no config option should ever exist for this unless there is a case in which code only works when put in a text and that issue is not easily fixed

jaylawl commented 6 years ago

@bensku : in my case, i love to "force-convert" variables of any kind into text for fail-safe comparison conditions. in a sense where i just wanna make sure the condition works as i intended.

turning this warning off should definitly be in skript's options. what is this now, turning into the way apple treats their customers á la "listen here you lil' shit, you'll do it how we want you to do it"? there are already options to disable certain warnings - so you gotta pick a side here: remove ALL those options and force skript's users into the way you would code, or continue letting users have a sense of being free to do what they want, the way they want to.

i just upgraded to the latest version, previously ran dev-32d. my first instinct was to go into the options to deactivate that warning message, obviously to no avail. on reloads the console is getting spammed as if there were a spambot on the server, i can't read anything. so, gonna downgrad for now.

/edit: thinking about this a bit further: if you (addressing involved developers) really believed, that this was a neccessary change that everybody needed to make to their scripts, then you would deny the scripts that have that allegedly redundant text conversion from loading/executing at all. but instead it's just a warning message. so do you really not believe it's neccessary, or why are you not going all the way with this?

i can only assume the overall intention of this is to reduce the amount of incoming support requests from people that don't really know what they're doing. but heck, just activate the warning message by default and explain the options.sk why you think it's a bad idea to turn that stuff off. eventually, there won't be any more people doing it. deez nuts tho

Blueyescat commented 6 years ago

@jaylawl it is a warning because the code is not wrong. Also it will not break your server when you update Skript before fixing them.

If you think you need to disable that warning, then give an example code please.

The only thing i could find is using it as a default null value. Like:

if "%arg-1%" is "<none>" or "closed":

Bust just an ugly code, you make the default value of the argument (or whatever) "closed" in this case.. anyway still can be replaced with these:

if (arg-1 otherwise "<none>") is "<none>" or "closed":
if "x%arg-1%" is "x<none>" is "xclosed":
jaylawl commented 6 years ago

@Blueyescat okay, so after i fix all my code to the desired vision of the Skript team, when can i expect the next Skript update that forces me to make major revisions to my code because the Skript team decided my code is ugly?

/edit: also, what about a compromise? if it's insisted on that this warning is important, how about make a config option that warns about this case once instead of repeatedly for each and every occurence. that way, people still get to see the message, but aren't (in-cases) spammed with it.

TheBentoBox commented 6 years ago

The point is that if the warning is made optional in any way, >95% of users will just disable/ignore it which makes adding it pointless. You made this somewhat clear yourself in your earlier reply:

in my case, i love to "force-convert" variables of any kind into text for fail-safe comparison conditions. in a sense where i just wanna make sure the condition works as i intended.

This is willful ignorance. If you need to forcibly convert a variable for the comparison to work properly, then that's something wrong with Skript which should be addressed and fixed. You should not need to forcibly reparse a string as a string in order to compare it to another string. If you prefer the forced conversion, you're saying you prefer performing unnecessary operations which at best do nothing and at worst slow down your scripts. And it's not like the syntax looks nicer than not converting it either. There is simply no reason that anyone should desire to do so and it needs to be forcibly instilled in the Skript community that it's something that they need not do because a ton of Skripters (myself included) are guilty of doing so all the time.

Just to follow up, imagine how ridiculous it would be in another language. Sure, in Skript saying if "%command%" is "home" doesn't look ridiculous because we're so used to it, but imagine it in some other language:

let command: string = "home";
if command.toString() === "home":
    # logic

That would be preposterous and if I was reading someone's code and saw them doing that I'd call them out immediately.

Blueyescat commented 6 years ago

Okay so you couldn't find a valid reason. Accept it and fix your code. Fixing your code is easier than discussing here 👍

jaylawl commented 6 years ago

@TheBentoBox the difference to other languages being, that they wouldn't "play code-police" unless the code didn't compile.

@Blueyescat you said earlier that the code is not wrong. now you're saying one needs to fix it.

Blueyescat commented 6 years ago

Means fix the warning.... good luck on fixing them 👋

jaylawl commented 6 years ago

@Blueyescat well thanks for the "fuck you". what a nice person you are

TheBentoBox commented 6 years ago

The difference to other languages is that they also don't have a relatively small community where many of the people who write the language also deal directly with the individual members of the community of newbie users writing poor quality code who need things explained to them on the daily :)

Skript is also very specifically touted as a "noob-friendly" language and a large portion of the community either only knows Skript or were introduced to programming by Skript. That means Skript is in a very unique place in that it should help enforce or explain programming principles wherever possible, much more so than most other languages.

So yes, this is annoying to fix, but between simple tools like find & replace and the errors listing exactly where the problem areas are, I was able to fix this in my ~130 scripts in about 10 or 15 minutes, and it makes your code better and is better for the future (for new people).

bensku commented 6 years ago

Development releases of Skript are not considered to have stable API. Breaking changes are possible, albeit rare; non-breaking changes are frequently made and sometimes reverted. Please note that old development releases are not supported by us, so you probably shouldn't support your scripts on them. Some of them are extremely buggy.

If significant amount of users were backing reversion of this change, I'd definitely consider it. However, this far I've not seen more than a few people being bothered by it too much. Pushing for better code is, sadly, more important in this case, in my opinion.

Pikachu920 commented 6 years ago

late to the party, but:

in my case, i love to "force-convert" variables of any kind into text for fail-safe comparison conditions. in a sense where i just wanna make sure the condition works as i intended. This warn won't show for variables anyway

turning this warning off should definitly be in skript's options. what is this now, turning into the way apple treats their customers á la "listen here you lil' shit, you'll do it how we want you to do it"? there are already options to disable certain warnings - so you gotta pick a side here: remove ALL those options and force skript's users into the way you would code, or continue letting users have a sense of being free to do what they want, the way they want to. The options that exist are for warnings which are correct in most cases but do have valid cases for the warning to be ignored. For example, the missing and or warning option is mostly for skript-mirror because stylistically and doesn't really look good there.

Skript isn't anything like Apple -- it's open source software. If you really disagree with some decision you can always make your own fork and nobody can stop you (assuming you comply with the license).

i just upgraded to the latest version, previously ran dev-32d. my first instinct was to go into the options to deactivate that warning message, obviously to no avail. on reloads the console is getting spammed as if there were a spambot on the server, i can't read anything. so, gonna downgrad for now.

Yeah, it sucks that the error hasn't been a thing ever since Skript has had strings but this argument doesn't really mean anything because its just people not wanting to deal with the annoyance of changing their scripts which, while understandable, is not really a good reason to avoid change in the grand scheme of things.

if you (addressing involved developers) really believed, that this was a neccessary change that everybody needed to make to their scripts, then you would deny the scripts that have that allegedly redundant text conversion from loading/executing at all. but instead it's just a warning message. so do you really not believe it's neccessary, or why are you not going all the way with this?

Like the other warnings, it is a warning because it is syntactically correct but stylistically wrong. Many other languages have the same warning, including Java which will print a compile time warn when you call String#toString on a string.

i can only assume the overall intention of this is to reduce the amount of incoming support requests from people that don't really know what they're doing. but heck, just activate the warning message by default and explain the options.sk why you think it's a bad idea to turn that stuff off. eventually, there won't be any more people doing it. deez nuts tho

It goes beyond that because I would hope the error helps scripters understand where and why %s are required which has been a source of confusion for a long time.