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

Feature Q-codes #56

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

Feature Q-codes #56

wants to merge 8 commits into from

Conversation

Sandarr95
Copy link

@Sandarr95 Sandarr95 commented Sep 7, 2020

So I redid the PR in its own branch.

A couple of things I need some opinion/guidance on:

  • Is the update to the README.md satisfactory now?
  • Is it okay to add the dependency on compojure or would you rather see an implementation without an additional dep?
  • What would make sense to return from the direct-reply handler? I'm not sure about {:status 200}

As for the tests, I've never written those. Interested to learn though, but that will take me a bit. It's probably not correct on the first try.

@Sandarr95 Sandarr95 mentioned this pull request Sep 7, 2020
@Otann
Copy link
Owner

Otann commented Sep 8, 2020

Is the update to the README.md satisfactory now?

Spot on 👌

Is it okay to add the dependency on compojure or would you rather see an implementation without an additional dep?

I would rather have a morse-compojure module, which can have compojure-specific handlers ;)
If you go down that road, I promise to port everything to pedestal even ;)

What would make sense to return from the direct-reply handler?

I think it makes sense, should be enough for Telegram

@Sandarr95
Copy link
Author

I hope the changes are understandable from the commit messages.
Looking forward to feedback on the tests, or anything else.

@Otann
Copy link
Owner

Otann commented Sep 10, 2020

👌

@Sandarr95
Copy link
Author

Tested direct reply as implemented currently against my own project by adding deps.edn in https://github.com/Sandarr95/morse/tree/testing/deps
Haven't had the chance to test the req->morse with webhooks yet.

@Sandarr95 Sandarr95 marked this pull request as ready for review September 10, 2020 18:39
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.

2 participants