Godot version:
Any
OS/device including version:
Any
Issue description:
What is the sense of String.erase( int position, int chars ) if it does not return anything?
https://docs.godotengine.org/en/stable/classes/class_string.html#class-string-method-erase
It changes the string :)
Seems like string in gdscript are indeed mutable ;)
PS: It is however weird as the documentation states that strings are supposed to be copy-on-write and all other functions do respect this convention
Looks like this is the only String method that modifies the String instead of returning a new one 馃
I'd vote for revising this method to make it immutable like other String methods. What should we rename it to?
Yeah, this is confusing for me. It is better when all functionality works the same way.
I'd vote for revising this method to make it immutable like other String methods. What should we rename it to?
Maybe something like: splice?
@MWFIAE Sounds good to me. The method would behave roughly like JavaScript's Array.splice()
then.
In Javascript, splice()
changes the original array whereas slice()
doesn't but both of them return array object. So, maybe we should go for something like slice()
to maintain immutability of String
.
@theoway We already have Array.slice()
which has different behavior, I fear giving a different behavior to String.slice()
would make it confusing.
@theoway We already have
Array.slice()
which has different behavior, I fear giving a different behavior toString.slice()
would make it confusing.
Indeed, it'd be confusing. Then it'd be a good option to implement something like splice
(like in Javascript)while maintaining string immutability.
Indeed, it'd be confusing. Then it'd be a good option to implement something like
splice
(like in Javascript)while maintaining string immutability.
Why not just update erase to return the new string and not mutate the string as a backwards incompatible break for 4.0? Since going forward you'd need to do something with 'erase' anyway. It seems cleaner then cluttering the API up with another method name.
Indeed, it'd be confusing. Then it'd be a good option to implement something like
splice
(like in Javascript)while maintaining string immutability.Why not just update erase to return the new string and not mutate the string as a backwards incompatible break for 4.0? Since going forward you'd need to do something with 'erase' anyway. It seems cleaner then cluttering the API up with another method name.
Makes sense. But the method name needs to be changed, as it won't be erasing anymore(after changing the method)
Upon thinking beyond the method itself, we have a similar method String.insert()
, which is immutuable and returns a new String
...
as it won't be erasing anymore
Neither insert
would insert a new substring into existing one, so perhaps it also has to be renamed? Not sure... 馃
Neither
insert
would insert a new substring into existing one, so perhaps it also has to be renamed? Not sure... 馃
Indeed. If anyone has suggestions as to what it should be renamed to, drop 'em here. I'll make a PR to change the erase()
to something like splice
and change the name of insert()
Maybe we should remove/unexpose String.erase()
entirely in 4.0? Mutable string methods can cause more trouble than they solve.
Maybe we should remove/unexpose
String.erase()
entirely in 4.0? Mutable string methods can cause more trouble than they solve.
That's a good idea. It can be replaced with a similar method, like splice(in Javascript), making sure that it doesn't mutate the string. How does that sound?
@theoway Isn't JavaScript's Array.splice()
the same as Array.slice()
but in a mutable form? Going by this question on Stack Overflow, I can't see a difference other than the mutability.
@theoway Isn't JavaScript's
Array.splice()
the same asArray.slice()
but in a mutable form? Going by this question on Stack Overflow, I can't see a difference other than the mutability.
Also, splice can add elements at the index from where old elements were removed.
So, in a way, it can do the job of insert()
& erase()
See this article: https://www.javascripttutorial.net/javascript-array-splice/
@theoway In this case, I guess it makes sense to implement an immutable version of String.splice()
.
@theoway In this case, I guess it makes sense to implement an immutable version of
String.splice()
.
Should I make the PR of implementation or make a proposal first?
@theoway I think you can open a pull request directly against the master
branch and remove String.erase()
at the same time.
@Calinou I had one more question. Since, we're implementing splice, it'll make insert()
redundant in gdscript string. I was wondering if we should get rid of it as well.
@theoway I guess it makes sense to remove String.insert()
as well in this case.
@Calinou Now there are many references to erase()
and insert()
throughout the project. So, should I remove these methods and make changes everywhere or add the new method(i.e. splice()
) while keeping these two(erase()
and insert()
), but removing their references from engine's scripting languages(gdscript, gdnative etc.)?
Most helpful comment
I'd vote for revising this method to make it immutable like other String methods. What should we rename it to?