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

feat(frontend/aiscript): MFMを考慮してテキストを操作する関数を追加 #15047

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

kakkokari-gtyih
Copy link
Contributor

@kakkokari-gtyih kakkokari-gtyih commented Nov 23, 2024

What

image

MFMを考慮してテキスト部分にだけ加工を加える関数を追加

Why

Fix #14975 (?)

Additional info (optional)

toStringの挙動により一部出力がおかしい
Related to misskey-dev/mfm.js#151, misskey-dev/mfm.js#125

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

@github-actions github-actions bot added the packages/frontend Client side specific issue/PR label Nov 23, 2024
@kakkokari-gtyih kakkokari-gtyih marked this pull request as ready for review November 23, 2024 07:17
@@ -4,6 +4,7 @@
-

### Client
- Feat: MFMを考慮してテキストを操作するAiScript関数を追加
Copy link
Contributor Author

@kakkokari-gtyih kakkokari-gtyih Nov 23, 2024

Choose a reason for hiding this comment

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

Suggested change
- Feat: MFMを考慮してテキストを操作するAiScript関数を追加
- Feat: MFMを考慮してテキストを操作するAiScript関数を追加
- 既知の問題として、操作の前後でMFMや一部の構文が意図せず変化することがあります

Copy link

codecov bot commented Nov 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 29 lines in your changes missing coverage. Please review.

Project coverage is 19.32%. Comparing base (d91a1be) to head (4eea36b).
Report is 8 commits behind head on develop.

Files with missing lines Patch % Lines
packages/frontend/src/scripts/aiscript/api.ts 0.00% 29 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #15047      +/-   ##
===========================================
+ Coverage    19.14%   19.32%   +0.17%     
===========================================
  Files          728      728              
  Lines       103800   103829      +29     
  Branches       993      991       -2     
===========================================
+ Hits         19876    20066     +190     
+ Misses       83368    83209     -159     
+ Partials       556      554       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sayamame-beans
Copy link
Member

replaceMfmだと、"MFMをreplaceする"ような印象を受けるので、別の名前の方が良いかもしれません…? (といっても代替案思いついていませんが)

@kakkokari-gtyih
Copy link
Contributor Author

といっても代替案思いついていませんが

これ募集中

@Sayamame-beans
Copy link
Member

Sayamame-beans commented Nov 23, 2024

Mk:replace_except_mfmとか…?
(ちなみにAiScript/MisskeyプラグインAPI辺りはスネークケースを使っているイメージあります(キャメルケースではなく))

@fruitriin
Copy link
Contributor

fruitriin commented Nov 23, 2024

image

ChatGPTに聞いてみたので参考になれば


ChatGPTが混乱しないようにMFMではなく「Markupされていないもの」と指示を出したので
適当に単語をMFMに変えるといい感じかも

@syuilo
Copy link
Member

syuilo commented Nov 23, 2024

replaceTextNodeとか

@fruitriin
Copy link
Contributor

MFMのAST的にはTextNodeは馴染みのある概念だけど、AiScriptユーザー的にはTextNodeが何を指してるかわからない気がする
(まあ決めの問題なのでなんでもいいとも思うけど)

@anatawa12
Copy link
Member

anatawa12 commented Nov 23, 2024

長くなるけどMFMであることとText対象であること両方名前に入れたほうが誤解しないだろうしってreplaceTextOfMfmとか?

TextNodeだとMFMであるってことが抜けちゃうんで

@fruitriin
Copy link
Contributor

fruitriin commented Nov 23, 2024

replacePlainText とか replaceNonMFMとかがすき


元の関数の挙動を全然理解してないで言ってた説

@fruitriin
Copy link
Contributor

そう考えると、replaceTextNodeが結構しっくり気がしてきたら

@anatawa12
Copy link
Member

anatawa12 commented Nov 23, 2024

replacePlainTextとかreplaceTextNodeはMFMを受け取る関数であるという(私は非自明だと思ってる)情報が欠落しちゃって私はあまり好きではない(TextNodeはmfm.js用語だけどmfmの一般単語ではないし)

@fruitriin
Copy link
Contributor

新しい関数を足すより挙動変更オプション足すのもありかも

mode: ExcludeMFMToken, ExcludeMFMScentence, ....

@salano-ym
Copy link
Contributor

MFMで名前空間を作るという手もあります
Mfm:replace_text

@Sayamame-beans
Copy link
Member

MFMで名前空間を作るという手もあります Mfm:replace_text

issueの方で言及のあった「それでいえばMFM解析をAiScriptで行えるとかなり幅広いことはできそう」みたいなものを実現する未来があるなら、そっち(Mfm名前空間を作る)の方が実際汎用性高そうですね…

@syuilo
Copy link
Member

syuilo commented Nov 24, 2024

元から名前空間は作る認識だった

@syuilo
Copy link
Member

syuilo commented Nov 24, 2024

ので自分的にはMfm:replaceTextNodeを推している

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/frontend Client side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hard Nyaize
6 participants