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

ユーザ統計のバグ修正 #397

Merged
merged 7 commits into from
Nov 24, 2023
Merged

ユーザ統計のバグ修正 #397

merged 7 commits into from
Nov 24, 2023

Conversation

tukeJonny
Copy link
Contributor

@tukeJonny tukeJonny requested review from eagletmt and okashoi and removed request for eagletmt November 24, 2023 07:57
@kfly8
Copy link
Contributor

kfly8 commented Nov 24, 2023

perl変更良さそうです!

Copy link
Member

@eagletmt eagletmt left a comment

Choose a reason for hiding this comment

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

for user in &users のままでも問題は無いんですが、できればこうしたいのでこの変更も入れてほしいです。

diff --git a/webapp/rust/src/main.rs b/webapp/rust/src/main.rs
index 315621c..be6d75d 100644
--- a/webapp/rust/src/main.rs
+++ b/webapp/rust/src/main.rs
@@ -1766,8 +1766,8 @@ struct UserStatistics {
 }
 
 #[derive(Debug)]
-struct UserRankingEntry<'a> {
-    username: &'a str,
+struct UserRankingEntry {
+    username: String,
     score: i64,
 }
 
@@ -1836,7 +1836,7 @@ async fn get_user_statistics_handler(
         .await?;
 
     let mut ranking = Vec::new();
-    for user in &users {
+    for user in users {
         let query = r#"
         SELECT COUNT(*) FROM users u
         INNER JOIN livestreams l ON l.user_id = u.id
@@ -1861,14 +1861,14 @@ async fn get_user_statistics_handler(
 
         let score = reactions + tips;
         ranking.push(UserRankingEntry {
-            username: &user.name,
+            username: user.name,
             score,
         });
     }
     ranking.sort_by(|a, b| {
         a.score
             .cmp(&b.score)
-            .then_with(|| a.username.cmp(b.username))
+            .then_with(|| a.username.cmp(&b.username))
     });
 
     let rpos = ranking

.fetch_all(&mut *tx)
.await?;

for livestream in livestreams {
Copy link
Member

Choose a reason for hiding this comment

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

コンパイル通ってないのでここを参照にしてください

Suggested change
for livestream in livestreams {
for livestream in &livestreams {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ありがとうございます 🙇
こちらで対応しました dcd03e8

@kfly8
Copy link
Contributor

kfly8 commented Nov 24, 2023

Perl、CIコケてますね。
出先なので、1時間後くらいに確認させてもらいます🙇

@kfly8
Copy link
Contributor

kfly8 commented Nov 24, 2023

https://github.com/isucon/isucon13/actions/runs/6978516358/job/18990868487#step:11:1

CIコケてるのは、変数名間違ってるのが原因みたいですね。

@tukeJonny
Copy link
Contributor Author

https://github.com/isucon/isucon13/actions/runs/6978516358/job/18990868487#step:11:1

CIコケてるのは、変数名間違ってるのが原因みたいですね。

ありがとうございます 🙇
こちらで対応しました 7f84431

Copy link
Member

@eagletmt eagletmt left a comment

Choose a reason for hiding this comment

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

Ruby、Rust は良さそうです。ランク算出で user 変数が被ってるのが気になるといえば気になりますが、まぁ Go 実装もそうなってるからな……

@okashoi
Copy link
Contributor

okashoi commented Nov 24, 2023

PHP みました問題ありません!

@kfly8 kfly8 requested review from kfly8 and removed request for okashoi November 24, 2023 10:14
Copy link
Contributor

@kfly8 kfly8 left a comment

Choose a reason for hiding this comment

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

Perl気になるところ微調整をしました!大丈夫そうです!

@kfly8 kfly8 requested a review from okashoi November 24, 2023 10:18
Copy link
Contributor

@YutaUra YutaUra left a comment

Choose a reason for hiding this comment

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

Node.js OK です!

@tukeJonny
Copy link
Contributor Author

ありがとうございます!
確認が取れたので、マージさせていただきます 🙇

@tukeJonny tukeJonny merged commit dc3ac9e into main Nov 24, 2023
7 checks passed
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.

6 participants