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

Implement FormattedString method #26

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

Conversation

Tiamenti
Copy link

@Tiamenti Tiamenti commented Jun 1, 2024

This PR adds a simplified implementation of the basic replace method from the JavaScript String class that preserves the original formatting after replacing text.

Problem:

bot.hears('test', async (ctx) => {
    const msg = fmt`I like the ${code('grammY')} framework`;
    const newText = msg.toString().replace('like', 'very love');

    const newMsg = new FormattedString(newText, msg.entities);

    await ctx.replyFmt(newMsg);
});

Screenshot_20240601_230429_Telegram

Solution after changes:

bot.hears('test', async (ctx) => {
    const msg = fmt`I like the ${code('grammY')} framework`;

    const newMsg = msg.replace('like', 'very love');

    await ctx.replyFmt(newMsg);
});

Screenshot_20240601_230652_Telegram

Copy link
Member

@KnorpelSenf KnorpelSenf left a comment

Choose a reason for hiding this comment

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

That is very cool stuff, I've long been wanting this but never took the time. Could we land both replace and replaceAll in the same pull request? :)

const index = this.text.indexOf(searchValue);

if (index !== -1 && index < newEntity.offset + newEntity.length) {
newEntity.offset = newEntity.offset + replaceValue.length - searchValue.length;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the PQ!

Please consider this counterexample test case in your next commit.

bot.command('start', async ctx => {
  const msg = fmt`I like the grammY ${code('framework')} very much`;

  const newMsg = msg.replace('framework', 'library');

  await ctx.replyFmt(newMsg);
});

Result:
Screenshot 2024-06-02 at 10 45 35 AM

@KnightNiwrem
Copy link
Collaborator

KnightNiwrem commented Jun 2, 2024

There is an increased complexity in replacing FormattedString as there are now 2 dimensions to consider. We can list down the following cases and discuss what the expected behavior should be.

Easy

Entity covers a full or partial word, occurs before replacement string, and does not intersect with replacement string.

Example: We love the grammY framework very much
Replacement: framework -> library
Expectation: We love the grammY library very much
Explanation: Neither the offset nor the length of the entity should be modified.
PQ currently handles this: Yes

Entity covers a full or partial word, occurs after replacement string, and does not intersect with replacement string.

Example: We love the grammY framework very much
Replacement: framework -> library
Expectation: We love the grammY library very much
Explanation: The length of the entity should not be modified. The offset needs to be moved as the replacement string has shifted left.
PQ currently handles this: Yes

Entity covers a full word and completely intersects with replacement string

Example: We love the grammY framework very much
Replacement: framework -> library
Expectation: We love the grammY library very much
Explanation: The natural expectation of replacing a bolded word with another, is to end up with a new bolded word. The length and offset of the entity should be modified.
PQ currently handles this: No

Medium

Entity covers a partial word, intersects with replacement string, and the replacement string length is greater than the entity length

Example: We love the grammY framework very much
Replacement: framework -> library
Expectation: We love the grammY library very much
Explanation: There isn't a conclusive answer on what it means to replace a word that is partially formatted. Here, the naive case of simply preserving the length of the entity may be "acceptable"
PQ currently handles this: Yes

Entity covers a partial word, intersects with replacement string, and the replacement string length is equal to the entity length

Example: We love the grammY framework very much
Replacement: framework -> libs
Expectation: We love the grammY libs very much
Explanation: There isn't a conclusive answer on what it means to replace a word that is partially formatted. Here, the naive case of simply preserving the length of the entity may be "acceptable". However, this case is definitely more questionable than the above case, as the formatting has gone from partial word to full word, and may affect future additional replace calls.
PQ currently handles this: Yes

Entity covers a partial word, intersects with replacement string, and the replacement string length is less than the entity length

Example: We love the grammY framework very much
Replacement: framework -> lib
Expectation: We love the grammY lib very much
Explanation: There isn't a conclusive answer on what it means to replace a word that is partially formatted. Though, this case is, again, definitely more questionable than the above case, as the formatting has gone from partial word to full word, plus the length of the entity has also changed. As usual, this may affect future additional replace calls.
PQ currently handles this: No

Hard

We won't go into much detail on what the hard cases are for the time being. In general, consider cases where phrases are formatted instead (either as a collection of full words only, or full and partial words).

@KnorpelSenf
Copy link
Member

I feel like it would be more intuitive to drop all formatting from the replaced strings. I don't see how we can assume that the old lengths or offsets make a lot of sense for newly inserted strings.

We love the grammY library a lot
with (library→framework) becomes
We love the grammY framework a lot

We love the grammY library a lot
with (lib→cont) becomes
We love the grammY contrary a lot

and so on.

We can accept new formatted strings as replacements instead.

We love the grammY library a lot
with (library→framework) becomes
We love the grammY framework a lot

Note that this suggestion means that we have to split and merge entities as necessary.

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

Successfully merging this pull request may close these issues.

3 participants