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

Update String docs to clarify some cases where copies are returned #91718

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aaronp64
Copy link
Contributor

@aaronp64 aaronp64 commented May 8, 2024

No description provided.

@aaronp64 aaronp64 requested a review from a team as a code owner May 8, 2024 15:38
@AThousandShips AThousandShips changed the title Updated String docs to clarify some cases where copies are returned Update String docs to clarify some cases where copies are returned May 8, 2024
@AThousandShips AThousandShips added this to the 4.x milestone May 8, 2024
@AThousandShips
Copy link
Member

All versions copy the data if they return a String, this should be clear from the const modifier IMO, it says that it doesn't modify the instance in the tooltip, so I'd say this is unnecessary, unless someone doesn't understand what the const description means, and if that's the case then that tooltip should be improved instead IMO

@aaronp64
Copy link
Contributor Author

aaronp64 commented May 8, 2024

Probably not necessary, but I do feel like it's a little clearer when looking at the documentation online, especially since some methods already state that they return a copy (for example, dedent says it returns a copy, but indent does not). The const part doesn't really stand out (at least with my browser settings - it's just gray on darker gray), so I could see it being missed if someone's just looking at the description of something that sounds like it could modify a string, like insert or replace.

Granted, it even says at the top of the page "every modification to a string returns a new String", which I missed before making this change. With that and the const, the information is definitely there, but I think it could be more consistent/obvious.

@AThousandShips
Copy link
Member

I'd say it would be useful to be consistent, but I feel these new descriptions are far more wordy, less clear. Changing the documentation text also erases any translation work done on them, which should be taken into account.

To me the new versions are harder to parse, and come at the cost of losing translation, but uniformity is good, though I'd prefer to remove the copy part rather than adding it

One alternative is to instead follow the style of erase, to me that's far easier to read and would change the existing descriptions far less, which is also technically more correct, a "copy" here IMO isn't relevant as it isn't really a copy, it's a new string with parts of the original kept

Maybe we can compare the number of styles for each type and compare, so we make as few changes as possible, any change here should be done in one organized step IMO, unifying all in one specific style so we settle on one solution, as there are at least three different styles now

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.

None yet

2 participants