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

Add text replacement methods #1755

Merged
merged 1 commit into from Jul 6, 2019

Conversation

pie-flavor
Copy link
Contributor

@pie-flavor pie-flavor commented Jan 30, 2018

These are very much needed, given the relative underpoweredness of TextTemplates.

@ryantheleach
Copy link
Contributor

ryantheleach commented Jan 30, 2018

@pie-flavor Is this still Work In Progress? if so reply with ~wip to mark your issue as Work In Progress or reply with ~qa to mark it as ready for Quality Assurance, if you wish it to be reviewed / tested by contributors / the community.

@Defman
Copy link

Defman commented Feb 6, 2018

Is Text not meant to be imutable: Represents an immutable tree-structure of formatted (text) components. Each instance consists of content and a list of children texts appended after the content of this text. The content of the text is available through one of the subclasses.

@ryantheleach
Copy link
Contributor

ryantheleach commented Feb 6, 2018

Sure, but it's a utility function for something that is inherently difficult to do yourself 100% correctly the first time.

Strings are immutable too, doesn't stop them having replace, but it's not something you would want to (ab)use all the time.

My 2c.

@dualspiral
Copy link
Contributor

I don't see a problem with it as long as the javadocs make it clear (in both the body and the returns statement) that the method returns a copy of the Text object with the specified replacements.

@pie-flavor
Copy link
Contributor Author

@The-Defman I'm confused - where's the issue? The original text never gets modified.

@dualspiral I'll add that, though originally I never thought it could be an issue - anyone using Text.replace has inevitably used String.replace which behaves the same way.

@randombyte-developer
Copy link

Can this be added to the next 7.x release or is that against semver?

@pie-flavor
Copy link
Contributor Author

No API methods get broken, so 7.x should be a valid target.

@stephan-gh stephan-gh removed their assignment Aug 10, 2018
@stephan-gh stephan-gh removed their request for review August 10, 2018 17:44
@gabizou
Copy link
Member

gabizou commented Aug 30, 2018

Needs to be updated since the text was refactored.

@Cybermaxke
Copy link
Contributor

@gabizou This targets API 7

@ImMorpheus
Copy link
Contributor

@Cybermaxke you need to change the target branch to stable-7 then

@pie-flavor pie-flavor changed the base branch from bleeding to stable-7 September 3, 2018 20:01
@pie-flavor
Copy link
Contributor Author

Oh good, the git history got clonked. Is this worth pursuing in its current state? Will there be another API 7 release?

@ImMorpheus
Copy link
Contributor

@pie-flavor Yes, please update and rebase

@pie-flavor
Copy link
Contributor Author

Looks all good from here.

@XakepSDK
Copy link

XakepSDK commented Mar 5, 2019

Maybe replace(Text, Text)?

@pie-flavor
Copy link
Contributor Author

Properly searching for a Text would require remembering the effective formatting of the text in addition to the Text's own formatting, and additionally parsing children, greatly complicating the algorithm. And there wouldn't really be a point in a Text argument that's only ever used via toPlain.

Copy link
Contributor

@ImMorpheus ImMorpheus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working as intended

@ImMorpheus ImMorpheus merged commit b931801 into SpongePowered:stable-7 Jul 6, 2019
@Lignium
Copy link
Contributor

Lignium commented Jul 11, 2019

You need to add these methods to API 8.

@pie-flavor pie-flavor deleted the feature/text-replace branch July 13, 2019 00:03
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