Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
UpdateEventの動作を変更する #447
base: main
Are you sure you want to change the base?
UpdateEventの動作を変更する #447
Changes from all commits
c10fd4c
3078036
fd96667
6a752b0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これより後ろの処理はグループが変わらなかったら行わなくても良い処理だと思うので、ここらへんで早期リターンするとよさそうです
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
変更前の主催者メンバー全員が変更後の主催者メンバーであるとき早期リターンします
具体的には
(変更前主催者メンバーの数) = (変更後主催者メンバーの数) - (変更後主催者メンバーの中で新規主催者メンバーの数)
で取ります
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currentEvent.GroupID == params.GroupID
で取ることを想定していましたThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
たしかにそっちのほうがわかりやすそうです
一方で、
(変更前主催者メンバーの数) = (変更後主催者メンバーの数) - (変更後主催者メンバーの中で新規主催者メンバーの数)
だとみたいな状況のときもできそうです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほど
ちょっと考えたけど本質的には
をチェックしたいから1つ目をboolでもって
if !newMemberAdded && len() == len()
みたいに書くと分かりやすいかも?ですThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if !newMemberAdded && len() == len()
とif currentEvent.Group.ID == params.GroupID
は同じ動作になりそうだと思ったのですがどこが違いますでしょうか。あと、一番最初にras0qさんが変更を入れてほしいとおっしゃられた場所と、実際に私が
(変更前グループメンバーの数) = (変更後グループメンバーの数) - (変更後グループメンバーの中で新規グループメンバーの数)
の変更を入れていた場所が違いました。There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
今の変更点で不味そうなところがあったら、教えていただきたいです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
返信が遅くなってすみません
(変更前グループメンバーの数) = (変更後グループメンバーの数) - (変更後グループメンバーの中で新規グループメンバーの数)
これなぜかずっと新規メンバーがいるかどうかを確認してると思ってたんですけど削除するメンバーがいるかどうかを確認してるんですね
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
とかにするといいのかな