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

Added functions for chatDescription #210

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cb-lock
Copy link

@cb-lock cb-lock commented Nov 3, 2020

  • Added methods for setting/getting the chat description
  • Enlarged the maxMessageLength to support common message lengths

- Added methods for setting/getting the chat description
- Enlarged the maxMessageLength to support common message lengths
@cb-lock
Copy link
Author

cb-lock commented Nov 3, 2020

Hello and thank you creating this wonderful library! I would like to contribute some functions that I found missing for my use case. Please let me know if there are any issues with adopting these changes.

I am happy to help and contribute!

@@ -125,7 +129,7 @@ class UniversalTelegramBot {
unsigned int waitForResponse = 1500;
int _lastError;
int last_sent_message_id = 0;
int maxMessageLength = 1500;
int maxMessageLength = 10500;
Copy link
Owner

Choose a reason for hiding this comment

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

I'm concerned about the impact this will have on memory, does it need to be so large?

Copy link
Author

Choose a reason for hiding this comment

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

Ok, this is certainly a point of discussion and this is probably application dependent. I created a chat bot using the 1500 length setting and found that it did not work at all because messages were incomplete rendering the reply useless. In effect he bot got stuck doing nothing so that I thought my code was bad. Debugging into it gave me the hint to modify the message size.

I suggest making this an obvious parameter for the library by moving it to private and adding at least a set method to make clear it is meant for application management. In addition, it could become a parameter of the contructor if you don't mind.

If you want to go ahead with this change, I can offer to make the necessary modification.

@witnessmenow
Copy link
Owner

Thanks for the PR.

As mentioned in the above comment, I'm concerned about how large the message size is and what impact that will have.

Also new features like this will require some examples to demo how to use it

@cb-lock
Copy link
Author

cb-lock commented Nov 8, 2020

Thanks for the PR.

As mentioned in the above comment, I'm concerned about how large the message size is and what impact that will have.

Also new features like this will require some examples to demo how to use it

Ok, I will add an example.

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.

None yet

2 participants