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

Q-Code #29

Closed
wants to merge 8 commits into from
Closed

Q-Code #29

wants to merge 8 commits into from

Conversation

Sandarr95
Copy link

I left it really simple for now, just 2 functions in qcodes.clj.
One will auto reply any text returned by the handler.
The other is middleware, causing the handler to work the same for polling and webhooks (and can also be made secure).

Let me know what you think.

Also had the idea to make a single function to start the bot, so if you provide a webhook it will use a server, but not sure how to make people choose what server they want to use.

@gilmarsoares-luizalabs
Copy link

@Sandarr95 Are you can create a unittest?

Copy link
Owner

@Otann Otann left a comment

Choose a reason for hiding this comment

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

Finally (in 2 years!) got to read about the naming behind this PR and really liked it.
It needs some polishing, maybe you can do it if you have time and energy, or I'd do it soon.

Q-Codes in Morse are shorthands for common patterns. `morse.qcodes/direct-reply` & `morse.qcodes/req-morse` are the Q-Codes already available. Here is an example usage of both.

```clojure
(ns look.anotherbadlynamedns
Copy link
Owner

Choose a reason for hiding this comment

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

the rest of the docs uses slightly different style of imports

[morse.handlers :as h]
[somehttpserver :as http]))

(def token "")
Copy link
Owner

Choose a reason for hiding this comment

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

Also the is no need repeat what was there before

@@ -0,0 +1,11 @@
{:paths ["src"]
Copy link
Owner

Choose a reason for hiding this comment

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

it is quite confusing to have two sources of dependencies, because people would need to keep them both up to date

Copy link
Author

Choose a reason for hiding this comment

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

I forgot about this PR as well, this was added to do some troubleshooting. I'll look into fixing up this PR again next weekend.

#{(s/tuple "getMe" ::getMe)
(s/tuple "sendMessage" ::sendMessage)})

(s/def ::request
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see it used anywhere, maybe left here by mistake?

@Sandarr95 Sandarr95 marked this pull request as draft September 7, 2020 08:20
@Sandarr95
Copy link
Author

Cleaned this up in #56

@Sandarr95 Sandarr95 closed this Sep 7, 2020
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