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.

Sign up for GitHub

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

Jump to bottom

お気持ちを述べた #27

Merged
merged 4 commits into from
Jul 18, 2020
Merged

お気持ちを述べた #27

merged 4 commits into from
Jul 18, 2020

Conversation

KAZYPinkSaurus
Copy link
Member

お気持ちを述べました

処理の理由を知りたい箇所が他にもあればコメントください

@KAZYPinkSaurus KAZYPinkSaurus added the 重要度：高 Priority A label Jun 23, 2020
@KAZYPinkSaurus KAZYPinkSaurus self-assigned this Jun 23, 2020
@KAZYPinkSaurus KAZYPinkSaurus linked an issue Jun 23, 2020 that may be closed by this pull request
ソースコード内に全体的にコメントを追加します。 #20
Closed
@KAZYPinkSaurus KAZYPinkSaurus added this to In progress in task list via automation Jun 23, 2020
@solaWat
Copy link
Member

solaWat commented Jun 23, 2020

ありがたいです。
追加分については後で検討します。

ソースコードに合わせて、コメントも英語が良いです。。。

@KAZYPinkSaurus
Copy link
Member Author

コメントアウト英語化いたしました~ (DeepLが)

solaWat
solaWat reviewed Jun 29, 2020
View reviewed changes
notify/notify.py Outdated
if img_path == "":
res = requests.post(line_notify_api, data=payload, headers=headers)
else:
# 画像
# image
Copy link
Member

@solaWat solaWat Jun 29, 2020 •
edited

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.

情報量殆どないので削除します｡

画像はラインに分析結果をポストするときに使おう思っています｡

Copy link
Member

Choose a reason for hiding this comment

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

画像はラインに分析結果をポストするときに使おう思っています｡

これをTODOみたいな感じでコメントに残しておいてください。

solaWat
solaWat approved these changes Jun 29, 2020
View reviewed changes
notify/notify.py Outdated
files = {"imageFile": open(img_path, "rb")}
res = requests.post(line_notify_api, data=payload, headers=headers, files=files)
return res


# The reason why I set the maximum number of displays to 20 is because I thought the notification would be too long if more than 20 classes were displayed.
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

Choose a reason for hiding this comment

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

classesってなに？　って純粋になる。
max_"lessens" なのに、その他にclassという別の概念があるの？　という混乱。

Copy link
Member Author

Choose a reason for hiding this comment

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

自分で英語を書きます｡🙇‍♂️

solaWat
solaWat reviewed Jun 29, 2020
View reviewed changes
notify/notify.py
@@ -31,13 +30,15 @@ def main(day, max_lessons=20, config_file="conf/config.txt"):
USER_NAME = config["USER INFO"]["NAME"]
TUTORS_NAME_ID_MAP = config["TUTORS"]
CONTENT_REC_PATH = f"/tmp/app/db/instead-db-tmp-for-{USER_NAME}.txt"

# The reason we allow arguments like today tomorrw is that we thought it would be difficult to use a number as an argument to say how many days from today.
# The reason for the numerical conversion is to use timedelta to calculate the date
Copy link
Member

Choose a reason for hiding this comment

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

良いコメント。

solaWat
solaWat reviewed Jun 29, 2020
View reviewed changes
notify/notify.py Outdated
@@ -31,13 +30,15 @@ def main(day, max_lessons=20, config_file="conf/config.txt"):
USER_NAME = config["USER INFO"]["NAME"]
TUTORS_NAME_ID_MAP = config["TUTORS"]
CONTENT_REC_PATH = f"/tmp/app/db/instead-db-tmp-for-{USER_NAME}.txt"

# The reason we allow arguments like today tomorrw is that we thought it would be difficult to use a number as an argument to say how many days from today.
Copy link
Member

Choose a reason for hiding this comment

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

うーん、横に長い。。。

個人的には「何日後」という指定が、「today」「tomorrow」よりdifficult to use という主張にあまり共感できない。
むしろ楽な気が。。。まあ、それは細かいことなので。

Copy link
Member Author

Choose a reason for hiding this comment

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

自分で英語を書きます｡🙇‍♂️

solaWat
solaWat reviewed Jun 29, 2020
View reviewed changes
notify/notify.py Outdated
if day == "" or day == "today":
days = 0
elif day == "tomorrow":
days = 1
else:
days = int(day)
# Since the past is unavailable, I thought it would be more convenient to specify the number of days from today
Copy link
Member

Choose a reason for hiding this comment

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

もうちょっと短文にできそう。日本語からの直訳だと、余計な言い回しが入って、かえってわかりづらいかも。
中学生並みの英文で良いので、SVCなどの言い切り型でポンポンとはめていくのが良いかもしれません。

レビューとは関係ないですが、今日から数えて何日後というモードと、カレンダーで何日というモードが設定によって切り替えられると便利かもしれません。

Copy link
Member Author

Choose a reason for hiding this comment

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

自分で英語を書きます｡🙇‍♂️自分で英語を書きます｡🙇‍♂️自分で英語を書きます｡🙇‍♂️

カレンダーモードは素敵｡ やりたい

solaWat
solaWat reviewed Jun 29, 2020
View reviewed changes
notify/notify.py
if day == "" or day == "today":
days = 0
elif day == "tomorrow":
days = 1
else:
days = int(day)
# Since the past is unavailable, I thought it would be more convenient to specify the number of days from today
date = datetime.today() + timedelta(days=days)
date = datetime.strftime(date, "%Y-%m-%d")
logger.info("Search date: %s", date)
Copy link
Member

Choose a reason for hiding this comment

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

この行の一行下の正規表現のやつって、なんのために使ってるんですか。

   date_re = re.compile(fr"[0-9]{{2}}:[0-9]{{2}}")

Copy link
Member Author

@KAZYPinkSaurus KAZYPinkSaurus Jul 9, 2020 •
edited

Choose a reason for hiding this comment

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

予約枠の時間抽出するためです｡ なので date_re -> time_reですね｡
コメント追加します｡

@solaWat
Copy link
Member

solaWat commented Jun 29, 2020

なんでここでファイルを開いているんですか。何か事情があるのですか。（俺が書いたところだけど。。。）

english-class-notify/notify/notify.py

Line 47 in ef6266c

try:

@solaWat
Copy link
Member

solaWat commented Jun 29, 2020

python構文に慣れてないだけだけど、

if yoyakuka_lessens:
    --
else
    continue

とした方が読みやすそう。

english-class-notify/notify/notify.py

Line 62 in ef6266c

if not yoyakuka_lessons:

solaWat
solaWat reviewed Jun 29, 2020
View reviewed changes
notify/notify.py Outdated
@@ -72,7 +75,9 @@ def main(day, max_lessons=20, config_file="conf/config.txt"):
if message != file_content:
with open(CONTENT_REC_PATH, mode="w") as fw:
fw.write(message)
# I thought it would be easier to see the capitalization conversions in TODAY TOMORROW than in today tomorrow
Copy link
Member

Choose a reason for hiding this comment

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

"I thought"とか"it would be"とか要らないかな。。。バシッと本質だけ書いて欲しい。。。

関係ないけどPythonNotifyて関数名、結構ひどいね。。。

Copy link
Member Author

Choose a reason for hiding this comment

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

もっと簡潔にします｡
PythonNotify....これは酷い

solaWat
solaWat reviewed Jun 29, 2020
View reviewed changes
notify/notify.py Outdated
res = PythonNotify(f"@{USER_NAME}" + message + day.upper(), TOKEN)
# The HTTP response status code is kept in a log to make it easier to respond to problems.
Copy link
Member

Choose a reason for hiding this comment

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

長い。。。

@KAZYPinkSaurus
Copy link
Member Author

英語を変更しましたー

@solaWat solaWat mentioned this pull request Jul 11, 2020
Closed
@KAZYPinkSaurus KAZYPinkSaurus merged commit 720e30f into develop Jul 18, 2020
task list automation moved this from In progress to Done Jul 18, 2020
@solaWat solaWat deleted the feature/comment-out branch July 18, 2020 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@solaWat solaWat solaWat approved these changes

Assignees

@KAZYPinkSaurus KAZYPinkSaurus

Labels
重要度：高 Priority A
Projects
task list
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ソースコード内に全体的にコメントを追加します。
2 participants
@KAZYPinkSaurus @solaWat