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

Add webhooks support (#23) and improve errors handling (#26) #29

Merged
merged 5 commits into from
Jul 19, 2020

Conversation

Dolfik1
Copy link
Owner

@Dolfik1 Dolfik1 commented Jul 19, 2020

No description provided.

This was referenced Jul 19, 2020
@Dolfik1 Dolfik1 changed the title Issue23and26 Add webhooks support (#23) and improve errors handling (#26) Jul 19, 2020
@Dolfik1 Dolfik1 merged commit db91ed3 into master Jul 19, 2020
@ForNeVeR
Copy link
Contributor

ForNeVeR commented Jul 30, 2020

I should note that combining business logic changes and formatting changes into one commit is not the best idea. This PR is hard to read.

@@ -42,6 +42,37 @@ Use Funogram.Api module to communicate with Telegram API with the help of reques
```
So, that is it! Use Intellisence-driven development approach to explore all Funogram features! For further learning you may take a look at sample Telegram bots built using this library: <a href="https://github.com/Dolfik1/Funogram/tree/master/Funogram.TestBot">Test Bot</a>, <a href="https://github.com/worldbeater/Memes.Bot/tree/master/Memes">Memes Bot</a>

# Getting updates via webhooks
If you want to use webhooks, you should start application with admin privileges.
Copy link
Contributor

@ForNeVeR ForNeVeR Jul 30, 2020

Choose a reason for hiding this comment

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

It is unclear why you need some "admin privileges". It could be a port requirement from lower 1024 (*nix only, then?) or some silly HTTPS.sys idiosyncrasy (Windows-only, then?); not every OS/environment even has a concept of an "administrator".

Could you please clarify?

Copy link
Owner Author

Choose a reason for hiding this comment

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

It seems that admin privileges required only for windows system for HttpListener. I cannot claim this since I have not tested it on other systems.

Error { Description = e.Message; ErrorCode = -1 }

| _ ->
Error { Description = "Unknown error"; ErrorCode = -1 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be a good idea to at least try to stringify this unknown response here? As I understand, this case could match if there's Ok x, but neither x.Result nor x.Description is set. Which would be odd, yes, but still possible according to the types, and probably even having somewhat useful string representation.

E.g. | other -> Error { Description = sprintf "Unknown error: %s" other; ErrorCode = -1 }

@Dolfik1
Copy link
Owner Author

Dolfik1 commented Sep 12, 2020

I should note that combining business logic changes and formatting changes into one commit is not the best idea. This PR is hard to read.

Ofcourse this is a bad practice. I am sometimes a little lazy to make separate PR's :)

@ForNeVeR
Copy link
Contributor

I was making this comment not to mock you, but to maybe a bit encourage to be less lazy. Other people're looking at your project, too. We're honestly interested in it, and we're watching. Every. Little. Step.

@Dolfik1 Dolfik1 deleted the issue23and26 branch June 13, 2022 08:22
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

3 participants