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

Change behavior of String.right #36180

Merged
merged 1 commit into from
May 20, 2021

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Feb 13, 2020

Follow-up to #28648

Before:

print("Hello world".right(3)) #lo world

After:

print("Hello world".right(3)) #rld

Basically opposite of left(), so is more intuitive and consistent.

I replaced all source occurrences of right to substr(), which has the same behavior as old right. btw, right was calling substr anyways, so every instance that used it might be few µs faster now 🤔 (heh)

@groud
Copy link
Member

groud commented Feb 13, 2020

It would be good to implement, with both left() and right(), a behavior with negative indices.
So that print("Hello world".right(-3)) Would print lo world. (Basically this avoid typing print("Hello world".right(len("Hello world")-3))

@KoBeWi
Copy link
Member Author

KoBeWi commented Feb 13, 2020

^
I added this as another commit (for now ofc).

EDIT:
Also mentioned it in docs.

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 29, 2021

I have problems rebasing this due to renamed/moved files. Any advice welcome >_>

@madmiraal
Copy link
Contributor

I've had a look at trying to rebase your PR and it's going to be difficult. There are over a year's worth of commits that, as you identify, include several files that have been renamed and others that have been completely refactored. Considering the number of lines being changed in this PR vs the number of conflicts, it may be easier to recreate it, or I can reopen #47434.

@KoBeWi KoBeWi force-pushed the I_broke_your_right branch from ab68ebe to 2bcad19 Compare April 4, 2021 18:00
@KoBeWi KoBeWi requested review from a team as code owners April 4, 2021 18:00
@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 4, 2021

Ok rebased. I manually copied my string changes and then modified other files from scratch. Also updated some tests.

@KoBeWi KoBeWi removed request for a team April 4, 2021 18:02
@KoBeWi KoBeWi removed request for a team and bojidar-bg April 4, 2021 18:02
@groud
Copy link
Member

groud commented Apr 6, 2021

I accepted the change, but I think it might have been good to keep calls to right instead of substr whenever possible. Likely by using the new behavior with negative indices.

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 6, 2021

Why though? Right() serves a different purpose now, messing with indicies will only cause confusion.

@groud
Copy link
Member

groud commented Apr 6, 2021

Because right is a lot more explicit compared to substr regarding what it does. substr, from it's name, does not indicate if you take the right or the left part of the string.

It's not a big deal anyway but IMHO, it would make the code easier to understand.

@akien-mga
Copy link
Member

akien-mga commented May 18, 2021

I accepted the change, but I think it might have been good to keep calls to right instead of substr whenever possible. Likely by using the new behavior with negative indices.

I wonder about this too, but IMO there's a big semantic shift with this PR and the calls to right before the PR meant something very different semantically to what right would convey from now on.

Before the PR, we'd do things like str.right(1) to express "drop the first character, give me the rest", which doesn't actually have much to do with the right part of the string - it's more about how many characters to cut from the left.

After this PR, to convey the same meaning, str.substr(1, -1) might be a better way to convey this "cut one char, give the rest" behavior. If you want to keep using right(), it would have to be str.right(str.length() - 1) i.e. "give me a string with all characters minus one starting from the right".

If what you suggested was to use str.right(-1), it would not give the same result as what str.right(1) did before the PR.

@akien-mga
Copy link
Member

Needs a rebase, otherwise I think it's good to go.

@KoBeWi KoBeWi force-pushed the I_broke_your_right branch from 2bcad19 to b185951 Compare May 20, 2021 21:08
@KoBeWi
Copy link
Member Author

KoBeWi commented May 20, 2021

Rebased again. And added some tests again (for negative arguments).

@akien-mga akien-mga merged commit bfe4af9 into godotengine:master May 20, 2021
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants