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

groupByをどうにかする #5529

Closed
AyaMorisawa opened this issue Oct 20, 2019 · 2 comments · Fixed by #13975
Closed

groupByをどうにかする #5529

AyaMorisawa opened this issue Oct 20, 2019 · 2 comments · Fixed by #13975
Labels
💚Refactor Rewriting code without changing behavior

Comments

@AyaMorisawa
Copy link
Contributor

src/prelude/array.tsのgroupBy関数について、名前に対して期待する挙動と実際の挙動が合わないっぽいので

@acid-chicken @mei23

元のコメント

https://github.com/syuilo/misskey/pull/5515#discussion_r336269681

acid_chicken

挙動はわかるんですが、groupBygroupByX の名前がちょっとややこしいかなと思ったりします。

mei23

既存のgroupByがgroupByじゃなくて
mei23@095032a
とかしたかったのですが
消したら消したでなにか言われそうなので別名にしてお茶を濁しました

acid_chicken

うーん、個人的には既存の groupBy の名前を変えた方がいいと思います。

既存の方は表現としては間違ってはいないんですが、groupBy という名前を冠してしまうと個人的には期待されている動作とは違うかなといった感じがあります。ですが、LINQ 等とは異なって SQL 風の命名を避けているとも言えそうなので、いっそ NG ワードにしてしまってもいいかもしれません。

mei23

既存のgroupByは名前を変えるべきだとは思いますし
自分のソースなら変えちゃいますが人と調整してまで変えるモチベーションはないです
その変めんどくさいからサクッと別名にしたのに…

一度exportで公開しちゃったものでfork先とかどこで使われてるかわからないのと
これはgroupByを変更するpull requestではないので
とりあえずここでは元のgroupByは触らないことにして
それは別のIssueとかでやればいいのではと

@AyaMorisawa AyaMorisawa added the 💚Refactor Rewriting code without changing behavior label Oct 20, 2019
@AyaMorisawa
Copy link
Contributor Author

preludeは、(少なくとも建前上は)アプリケーションとは関連しないコードなので、prelude側から見てどんなデータ構造が欲しいのかは分からないはずというのがあります。分類するという処理と、分類したあとにできる要素を集めてデータ構造を構築するという処理を、うまく分離できれば、必要なデータ構造ごとに構築処理を記述することはできそうです。(ただ、そこまでするほど、必要なデータ構造が多くあるかは未確認)

@KisaragiEffective
Copy link
Collaborator

triage: 生きている

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💚Refactor Rewriting code without changing behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants