Skip to content

Conversation

@m-star18
Copy link
Member

Overview

プロジェクトが完成したかを判別して, いい感じに完成したURLを返してくれるAPiを作った

Issue number

Close #48

How to check the revision

Points for Review

Remarks

@m-star18 m-star18 self-assigned this Dec 17, 2022
@m-star18 m-star18 added the type:feature Feature Request label Dec 17, 2022
Copy link
Member

@kathmandu777 kathmandu777 left a comment

Choose a reason for hiding this comment

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

お願いします!!

Comment on lines +29 to +33
cls,
request: Request,
id: str,
text_messages_limit: int,
image_messages_limit: int,
Copy link
Member

Choose a reason for hiding this comment

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

formatter通ってます?
前回通してなかったのかなー?

Comment on lines 115 to 119
if (project.top_text and
project.top_image_url and
project.spotify_uri and
text_message_count >= 5 and
image_message_count >= 5):
Copy link
Member

Choose a reason for hiding this comment

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

formatter通ってなさそう

Copy link
Member

Choose a reason for hiding this comment

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

Projectにpropertyとして、can_publishをもたせようか。
APIの方にドメイン知識?を書くのはあまり良くないと思う。あんまり詳しくないからしらんけど。

image_message_count >= 5):
project.is_publish = True
else:
project.is_publish = False
Copy link
Member

Choose a reason for hiding this comment

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

save処理走らないなら、フラグ書き換えの意味はないと思います

Copy link
Member Author

Choose a reason for hiding this comment

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

これ, ifのsave処理走らせてると思う

Copy link
Member

Choose a reason for hiding this comment

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

elseの場合は、exceptionがraiseされるから、sync_to_async(project.save)() 実行されないと思う

Copy link
Member Author

Choose a reason for hiding this comment

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

その通りだ、修正します

][:limit]

async def get_image_message_count(self) -> int:
return MessageImage.objects.count()
Copy link
Member

Choose a reason for hiding this comment

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

上の定義と同じ内容のはずだけど書き方違うね

Copy link
Member Author

Choose a reason for hiding this comment

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

直します

return [project.id for project in Project.objects.all()]

@classmethod
async def post_publication_url(cls, request: Request, project_id: str) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

publish で良さそうだけどどう思う?

Copy link
Member Author

Choose a reason for hiding this comment

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

いいと思う

@kathmandu777
Copy link
Member

masterにrebase必要ですね

Copy link
Member

@kathmandu777 kathmandu777 left a comment

Choose a reason for hiding this comment

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

formatte走らない問題は解決したいな

image_message_count >= 5):
project.is_publish = True
else:
project.is_publish = False
Copy link
Member

Choose a reason for hiding this comment

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

elseの場合は、exceptionがraiseされるから、sync_to_async(project.save)() 実行されないと思う

@kathmandu777
Copy link
Member

conflictしてるから、rebase or merge必要

@m-star18
Copy link
Member Author

formatter問題はたしかに解決したい
rebaseについては#65 を先にmergeしてからやりたいかな(そっちともConflictしてるだろうから)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:feature Feature Request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

project 公開しているかのフラグ

3 participants