✨ add FilterDuration and replace FilterTime in InitPostEvent… #544

iChemy · 2024-06-18T04:38:45Z

iChemy · 2024-06-18T04:45:04Z

ある期間中(例: 今日中)にイベントが開催される瞬間があるかどうかをフィルターするために
func FilterDuration(min, max time.Time) Expr
を定義しこれを traQ にメッセージをポストする部分に適応させた．

utils パッケージ内で func addOr(lhs, rhs Expr) Exprというローカル関数を定義し func FilterDuration(min, max time.Time) Expr 内で用いている

traQに送信されるメッセージに時刻だけでなく日付の情報もわかるようにした(例)

## 06/20(Thu) の進捗部屋
本日は予約を取っていないようです。
## 06/20(Thu) 開催予定のイベント
本日開催されるイベントは、
- [イベント1](origin/events/61f5219b-ffb7-4546-9b6d-7ac1a9367c19) 06/19:13:00 ~ 06/23:13:00 @Discord 
- [イベント2](origin/events/72173508-348d-4021-8d0e-557912a09728) 06/20:12:00 ~ 06/20:18:00 @Discord

ras0q

ありがとうございます！
幾つか書きました

ras0q · 2024-06-18T05:36:34Z

domain/filter/shorthand.go

+// 期間より前に始まり期間より後に終わる
+// time_start < min AND time_end >= max
+//
+// min < max であるべき


これは直接コードで!min.IsZero() && !max.IsZero()であることを確認した上で時系列を判定するとよさそうです

ras0q · 2024-06-18T05:38:16Z

domain/filter/shorthand.go

+// time_start < min AND time_end >= max
+//
+// min < max であるべき
+func FilterDuration(min, max time.Time) Expr {


最近minとmaxがbuiltinの関数名に追加されたので他の名前にしておくと余計な誤解を招かなくてよいです(動きはするんですが)

since, untilなど

ras0q · 2024-06-18T05:41:17Z

domain/filter/shorthand.go

+		Rhs:     endAfterMax,
+	}
+
+	return addOr(addOr(startIn, endIn), throughout)


addOr使わずに直接書いた方が見やすそう(この関数内のAndはそうしてるし)

ras0q · 2024-06-18T05:47:50Z

utils/cron.go

-			eventMessage += fmt.Sprintf("- [%s](%s/events/%s) %s ~ %s @%s %s\n", event.Name, origin, event.ID,
-				event.TimeStart.In(tz.JST).Format("15:04"), event.TimeEnd.In(tz.JST).Format("15:04"),
+			eventMessage += fmt.Sprintf("- [%s](%s/events/%s) %s:%s ~ %s:%s @%s %s\n", event.Name, origin, event.ID,
+				event.TimeStart.In(tz.JST).Format("01/02"), event.TimeStart.In(tz.JST).Format("15:04"), event.TimeEnd.In(tz.JST).Format("01/02"), event.TimeEnd.In(tz.JST).Format("15:04"),


Format("01/02:15:04")でよさそう
あと日付と時間の区切りに:を見るのはあまり見ないので空白区切りがいいと思います

ras0q · 2024-06-18T05:51:02Z

domain/filter/shorthand.go

+// 期間中に始まる
+// time_start >= min AND time_start < max
+//
+// 期間中に終わる
+// time_end >= min AND time_end < max
+//
+// 期間より前に始まり期間より後に終わる
+// time_start < min AND time_end >= max


これをORで繋ぐことを明記すると良いと思います

Suggested change

// 期間中に始まる

// time_start >= min AND time_start < max

//

// 期間中に終わる

// time_end >= min AND time_end < max

//

// 期間より前に始まり期間より後に終わる

// time_start < min AND time_end >= max

// 以下のいずれかを満たす

// - 期間中に始まる (time_start >= min AND time_start < max)

// - 期間中に終わる (time_end >= min AND time_end < max)

// - 期間より前に始まり期間より後に終わる (time_start < min AND time_end >= max)

iChemy · 2024-06-20T13:18:59Z

修正してみました 🙇
お時間ある際に確認お願いします

ras0q

レビューしました！
今回のレビューはFilterDurationに関するものだけなのでまあ1コミットでもいいんですが、Refactoring codeみたいなコミットを打つと1コミットの粒度が大きくなってしまうので、1コミットでは最小単位の変更を記録してコミットを分割してくれると助かります！

ras0q · 2024-06-20T22:17:23Z

domain/filter/shorthand.go

@@ -93,3 +93,78 @@ func AddAnd(lhs, rhs Expr) Expr {
 		Rhs:     rhs,
 	}
 }
+
+// 以下のいずれかを満たす
+// * 期間中に始まる (time_start >= min AND time_start < max)


ここら辺のコメントもsince,untilにしてほしいです

ras0q · 2024-06-20T22:22:19Z

domain/filter/shorthand.go

+// * 期間中に終わる (time_end >= min AND time_end < max)
+// * 期間より前に始まり期間より後に終わる (time_start < min AND time_end >= max)
+//
+// since < until でない場合 nil を返す


nilを返すより返り値にエラーを追加してエラーを返すのが良さそう

InitPostEventToTraQ 内における filter.FilterDuration のエラーハンドリングですがどうするのが適切でしょうか?
同関数内の repo.GetAllRooms や repo.GetAllEvents はエラー値は利用されてないのですが filter.FilterDuration も同様にエラー値は無視するという形が良いかと思っています．

こんな感じ

expr, _ := filter.FilterDuration(now, tomorrow) events, _ := repo.GetAllEvents(expr)

cron jobを作ってるからerrorが返せないのか

err := RequestWebhook(message, secret, channelID, webhookID, 1) if err != nil { fmt.Println(err) }

の部分みたいにprintしておくだけでもやっておくと良いと思います

ras0q · 2024-06-20T22:23:38Z

domain/filter/shorthand.go

+		}
+	}
+
+	startAfterMin := &CmpExpr{


ここもstartAfterSinceとかにしたいが流石に文法に違和感があるのであまりいい名前が思いつかない
変に変数名付けずにstartInの中に直接入れてもそこまで分かりにくくはならないとも思ったり

iChemy · 2024-06-23T06:12:39Z

新しく

	if !since.IsZero() && !until.IsZero() {
		if since.After(until) {
			return nil, errors.New("invalid time range")
		}
	}

を返すようにしました

書いてて思ったんですが
IsZero()の条件のところいらない
かなと思ったのですがどう思いますか?

こんな感じ

		if since.After(until) {
			return nil, errors.New("invalid time range")
		}

(ただsince と until の前後関係を確認するだけの方がこの関数に想定される挙動な気がしています)

ras0q · 2024-06-23T20:26:08Z

domain/filter/shorthand.go

+	startBeforeMin := &CmpExpr{
+		Attr:     AttrTimeStart,
+		Relation: Less,
+		Value:    since,
+	}
+	endAfterMax := &CmpExpr{
+		Attr:     AttrTimeEnd,
+		Relation: GreterEq,
+		Value:    until,
+	}
+	// 期間より前に始まり期間より後に終わる
+	throughout := &LogicOpExpr{
+		LogicOp: And,
+		Lhs:     startBeforeMin,
+		Rhs:     endAfterMax,
+	}


ここも上と同様throughoutの中に直接書いちゃって良さそうです

ras0q · 2024-06-23T20:35:41Z

元のFilterTimeの実装を見るとIsZeroのときは時刻を指定しないということを意味してるようですね(knoQでは)
そうなるとそこのIsZeroはいらないんですが、別で

since,untilがどっちも指定されている(！IsZero())ときは今の3つの条件
sinceのみが指定されているときはsince<=終了時刻
untilのみが指定されているときは開始時刻<=until
どちらも指定されていない時はnil(条件なし)

を返す必要がありそうです

IsZero()の条件のところいらない
かなと思ったのですがどう思いますか?

iChemy · 2024-06-24T06:23:12Z

お願いします 🙇

ras0q · 2024-07-01T08:01:15Z

domain/filter/shorthand.go

-			return nil, errors.New("invalid time range")
-		}
+	if since.IsZero() && until.IsZero() {
+		return nil, nil


ここはエラーを返したい

嘘です
エラーはnilでいいけどExprはnilではなくて空の&CmpExpr{}を返すのがよさそう

ras0q

1件レビューしましたがそれ治したらマージしてOKです:thumbsup:

✨ add FilterDuration and replace FilterTime in InitPostEventToTraQ

8280e0a

iChemy requested review from Nzt3-gh and ras0q June 18, 2024 04:46

ras0q requested changes Jun 18, 2024

View reviewed changes

♻️ refactoring code

a2b852e

ras0q requested changes Jun 20, 2024

View reviewed changes

🎨 add error for returning value of FilterDuration

68fa95b

ras0q reviewed Jun 23, 2024

View reviewed changes

🎨 add zero time handling

e80bc38

ras0q requested changes Jul 1, 2024

View reviewed changes

ras0q approved these changes Jul 1, 2024

View reviewed changes

🎨 change nil return to empty struct

0b24a6a

iChemy merged commit a9a665d into main Jul 1, 2024
2 checks passed

iChemy deleted the fix/issue541 branch July 1, 2024 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ add FilterDuration and replace FilterTime in InitPostEvent… #544

✨ add FilterDuration and replace FilterTime in InitPostEvent… #544

iChemy commented Jun 18, 2024

iChemy commented Jun 18, 2024

ras0q left a comment

ras0q Jun 18, 2024

ras0q Jun 18, 2024

ras0q Jun 18, 2024

ras0q Jun 18, 2024

ras0q Jun 18, 2024

iChemy commented Jun 20, 2024

ras0q left a comment

ras0q Jun 20, 2024

ras0q Jun 20, 2024

iChemy Jun 21, 2024

ras0q Jun 21, 2024

ras0q Jun 20, 2024

iChemy commented Jun 23, 2024 •

edited

Loading

ras0q Jun 23, 2024

ras0q commented Jun 23, 2024 •

edited

Loading

iChemy commented Jun 24, 2024

ras0q Jul 1, 2024

ras0q Jul 1, 2024

ras0q left a comment

✨ add FilterDuration and replace FilterTime in InitPostEvent… #544

✨ add FilterDuration and replace FilterTime in InitPostEvent… #544

Conversation

iChemy commented Jun 18, 2024

iChemy commented Jun 18, 2024

ras0q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iChemy commented Jun 20, 2024

ras0q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iChemy commented Jun 23, 2024 • edited Loading

Choose a reason for hiding this comment

ras0q commented Jun 23, 2024 • edited Loading

iChemy commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ras0q left a comment

Choose a reason for hiding this comment

iChemy commented Jun 23, 2024 •

edited

Loading

ras0q commented Jun 23, 2024 •

edited

Loading