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

fix bugs in run.py and constants.py #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

MahtabRanjbar
Copy link

  • delete documantation of send_message function in src/run.py
  • delete bot.run() in src/run.py.
  • update constants.py to add keys.exit into keyboards.

src/run.py Outdated
Comment on lines 50 to 52
"""
Send message to telegram bot.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're deleting a wrong documentation, you should replace it with the correct one.

Copy link
Author

Choose a reason for hiding this comment

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

is it ok to write '"" send message to telegram user""" ?

Copy link

Choose a reason for hiding this comment

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

I think yes. It's okay to replace it with correct docstring.

@@ -59,4 +57,4 @@ def send_message(self, chat_id, text, reply_markup=None, emojize=True):
if __name__ == '__main__':
logger.info('Bot started')
nashenas_bot = Bot(telebot=bot)
nashenas_bot.run()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not right. You're just deleting the run part.

Copy link
Author

Choose a reason for hiding this comment

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

that part was extra. we add bot.infinity_pulling in init and we do not have run method in our class. out bot start running when initialized

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. So nashenas_bot name should be fixed cause this is a template bot. Can you change it to

logger.info('Bot started...')
Bot(telebot=bot)

Copy link
Author

Choose a reason for hiding this comment

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

yea that's right. I will do that

@arf1372
Copy link

arf1372 commented Dec 19, 2021 via email

@hejazizo
Copy link
Collaborator

hejazizo commented Dec 19, 2021

An important thing is “break up each change into individual commits”. Here I see three different changes. One, doc fix; Second, adding a functionality and, third, fixing a (maybe redundant) line in code. Please break them up to three different commits. Sent from Mailhttps://go.microsoft.com/fwlink/?LinkId=550986 for Windows From: @.> Sent: Monday, December 20, 2021 01:37 To: @.> Cc: @.> Subject: [pytopia/project-template-telegram-bot] fix bugs in run.py and constants.py (PR #1) * delete documantation of send_message function in src/run.py * delete bot.run() in src/run.py. * update constants.py to add keys.exit into keyboards. You can view, comment on, or merge this pull request online at: #1 Commit Summary * a47ab3e<a47ab3e> fix bugs in run.py and constants.py File Changes (2 fileshttps://github.com/pytopia/project-template-telegram-bot/pull/1/files) * M src/constants.pyhttps://github.com/pytopia/project-template-telegram-bot/pull/1/files#diff-42d0f00c4780305311eae71fecca993a0523587cb4156e039cabd0d1ad8dbbb8 (2) * M src/run.pyhttps://github.com/pytopia/project-template-telegram-bot/pull/1/files#diff-525565c956f875698bc74ac798b474aa561f4570464b770434888cfbf34bde48 (6) Patch Links: * https://github.com/pytopia/project-template-telegram-bot/pull/1.patch * https://github.com/pytopia/project-template-telegram-bot/pull/1.diff — Reply to this email directly, view it on GitHub<#1>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABOG2U3JC7UVJIJYQSISSC3URZJQRANCNFSM5KMLEDDQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you are subscribed to this thread.Message ID: @.>

Agreed Alireza, but this these are very small fixes, I think it's ok to merge them into on PR fix commit.

@MahtabRanjbar
Copy link
Author

@arf1372
Thanks for your good advice. I will try to observe that.

@arf1372
Copy link

arf1372 commented Dec 19, 2021

An important thing is “break up each change into individual commits”. Here I see three different changes. One, doc fix; Second, adding a functionality and, third, fixing a (maybe redundant) line in code. Please break them up to three different commits. Sent from Mailhttps://go.microsoft.com/fwlink/?LinkId=550986 for Windows From: @.> Sent: Monday, December 20, 2021 01:37 To: _@**._> Cc: _@.> Subject: [pytopia/project-template-telegram-bot] fix bugs in run.py and constants.py (PR #1) * delete documantation of send_message function in src/run.py * delete bot.run() in src/run.py. * update constants.py to add keys.exit into keyboards. You can view, comment on, or merge this pull request online at: #1 Commit Summary * a47ab3e<a47ab3e> fix bugs in run.py and constants.py File Changes (2 fileshttps://github.com//pull/1/files) * M src/constants.pyhttps://github.com//pull/1/files#diff-42d0f00c4780305311eae71fecca993a0523587cb4156e039cabd0d1ad8dbbb8 (2) * M src/run.pyhttps://github.com//pull/1/files#diff-525565c956f875698bc74ac798b474aa561f4570464b770434888cfbf34bde48 (6) Patch Links: * https://github.com/pytopia/project-template-telegram-bot/pull/1.patch * https://github.com/pytopia/project-template-telegram-bot/pull/1.diff — Reply to this email directly, view it on GitHub<#1>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABOG2U3JC7UVJIJYQSISSC3URZJQRANCNFSM5KMLEDDQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you are subscribed to this thread.Message ID: @.**_>

Agreed Alireza, but this these are very small fixes, I think it's ok to merge them into on PR fix commit.

Uh, I think having separate commit for each change worth it, specially when dealing with "git bisect" and "git blame" stuff.
So, I still insist on breaking commits into fine grained changes per commit.

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.

3 participants