Skip to content
This repository has been archived by the owner on Jul 9, 2022. It is now read-only.

Reply message feature & Some bug fixes #721

Merged
merged 18 commits into from
Aug 19, 2019
Merged

Reply message feature & Some bug fixes #721

merged 18 commits into from
Aug 19, 2019

Conversation

BadAimWeeb
Copy link
Contributor

@BadAimWeeb BadAimWeeb commented Apr 13, 2019

took 1 week to make this, but this feature is so easy to make
kill me plz

EDIT: also including #698, that pr is getting old

@HossamMohsen7
Copy link
Contributor

Attachments need to be formatted correctly

Copy link
Contributor

@hong4rc hong4rc left a comment

Choose a reason for hiding this comment

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

Too many duplications replyMessage with sendMessage

@Schmavery
Copy link
Owner

Is there a way to add this without copying most of sendMessage?
In the past, this sort of duplication has been a sign that maybe it should just be integrated as a different type of message in sendMessage.

@BadAimWeeb
Copy link
Contributor Author

sorry for late response

@BadAimWeeb BadAimWeeb mentioned this pull request Jul 1, 2019
@PixelHir
Copy link

bump

@BadAimWeeb BadAimWeeb mentioned this pull request Jul 26, 2019
@acenturyandabit
Copy link

Additional bump :)

@BadAimWeeb
Copy link
Contributor Author

@Schmavery

@PixelHir
Copy link

@Schmavery please, check this out
I had to create a forked version of the repo just to get the new stuff because this one is way too old

@BadAimWeeb BadAimWeeb changed the title Reply message feature. Reply message feature & Some bug fixes Aug 9, 2019
@BadAimWeeb
Copy link
Contributor Author

@Schmavery bump

@@ -303,7 +303,7 @@ module.exports = function(defaultFuncs, api, ctx) {
cb();
}

return function sendMessage(msg, threadID, callback) {
return function sendMessage(msg, threadID, callback, replyToMessage) {
Copy link
Owner

Choose a reason for hiding this comment

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

The callback is supposed to be optional, but this makes it so that you can't pass a reply without a callback.

DOCS.md Outdated
@@ -1372,7 +1428,7 @@ __Arguments__
---------------------------------------

<a name="sendMessage"></a>
### api.sendMessage(message, threadID[, callback])
### api.sendMessage(message, threadID[, callback[, messageID]])
Copy link
Owner

Choose a reason for hiding this comment

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

What does it mean for the [[]] to be nested? That you can't pass messageId without callback? Why?

Copy link
Owner

@Schmavery Schmavery left a comment

Choose a reason for hiding this comment

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

I don't have a lot of time for this library these days but I try to merge obvious bug fixes when I see them. I avoid merging new features because I don't have time to fix them when they break and usually people who make them can't/won't support them afterwards. I can try to make an exception here because it seems like you've been pretty involved.

Combining bug fixes with reformatting and extra features is usually a good recipe for making it hard for me to easily review and merge.

The fastest way to get something merged:

  • Make a low-impact bugfix
  • Describe behaviour before and after the change
  • If possible, have another user verify that the fix works on their machine.

I took some time to review this PR and found an issue in the arguments of sendMessage. If you clear that up I'll merge this. Thanks for your efforts.

@BadAimWeeb
Copy link
Contributor Author

ok, fixed that

Copy link
Owner

@Schmavery Schmavery left a comment

Choose a reason for hiding this comment

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

Thanks

@Schmavery Schmavery merged commit 9a698d7 into Schmavery:master Aug 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants