-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
help: choose pastebin service for .help output #1451
Conversation
In regards to the caching issue: Sopel won't re-read a config file after editing manually (that I know of). The main way to change the config on the fly is to use things like One solution could be to store the current pastebin provider right next to the cached URL, and check if the current |
@HumorBaby What I could do is change |
@uint, yep 😄 |
On rereading your post, that's exactly what you suggested ;) There goes my reading comprehension. But yeah. I'll add a commit soon. |
hastebin.com had an interruption a few days ago, posting was returning a 503 error. I wonder if it would be a good idea to - if there is no service configured - randomly pick a service, and try another one if there is an error. |
After 6.6.4 is released and its changes merged to Also, ptpb is shut down (semi-?)permanently (ptpb/pb#246), so it would be good to remove that option. Maybe replace it with clbin? |
@uint *poke* |
Rebased and "squashed" the whitespace-fix commits (not actually squashed, since both batches of fixed applied to multiple commits each). Next will be removing ptpb and adding… something else. Like clbin. |
(Edited/rebased to switch default from ptpb -> clbin, and whitespace fixed, by dgw) Co-Authored-By: dgw <dgw@technobabbl.es>
And ptpb->clbin default change: done |
(Rebased, default changed from ptpb->clbin, 0x0 switched to HTTPS, and whitespace fixed by dgw) Co-Authored-By: dgw <dgw@technobabbl.es>
(Rebased, default changed from ptpb->clbin, 0x0 switched to HTTPS, and whitespace fixed by dgw) Co-Authored-By: dgw <dgw@technobabbl.es>
(Rebased and whitespace fixed by dgw) Co-Authored-By: dgw <dgw@technobabbl.es>
(Rebased and whitespace fixed by dgw) Co-Authored-By: dgw <dgw@technobabbl.es>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One (optional) change requested for consistency and two (I would like to say required, at least in one form or another) changes requested for proper error handling of the low-level socket
for termbin.
The overarching theme of this review is.... 🎰 error handling!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imagine this was a part of the previous review... 🍄
All `requests.post()` calls moved to a wrapper that catches all the appropriate exception types and re-raises them as `PostingException` to simplify the error handling across callers. Termbin socket operations wrapped in a try/except, which will re-raise any error as a `PostingException` just like any other supported service.
Tomasz Kurcz definitely contributed enough code to be included here. Also took the liberty of including myself, since I'm rewriting a fair chunk of the new pastebin code.
Sometimes it depends on whether you called the API over HTTP or HTTPS. Sometimes the site just wants to save on HTTPS overhead by returning a URL with `http://`, hoping nobody will notice/care and correct it. We always call over HTTPS, but we'll make sure any valid response is acceptable (with or without the `s`) and enforce HTTPS before output.
`_pastebin_providers` -> `PASTEBIN_PROVIDERS` `s` (for socket object) -> `sock`
@HumorBaby Did a pretty big overhaul of error handling, and a few other little things while I was at it. 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nitpicks. But I approve otherwise.
This is just "by convention" stuff, nothing substantive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🏆
Misc. notes: At first, I was worried that @dgw improperly capitalized Hastebin 😱, but alas, since it seems to be a descendant of Haste, I have now come around to accepting the current capitalization.
Hey. Sorry for not having been responsive! Pretty busy and I... probably should configure e-mail notifications. Ahem. |
@uint You got us 90% of the way there! Most of what I did was just sanding off rough edges. 🙌 Without your initial set of commits, this feature would still be "Patches Welcome", but it's done! As for non-pastebin help output, we are tracking that in #1212. I think @RustyBower is working on something for that, unless he changed his mind. Feel free to join us in the #sopel IRC channel on freenode if you want to avoid duplicating effort on any future contributions. And thank you again for this PR! |
Description
I've created a new config section
help
with theoutput
setting.output
can currently be set to:"ptpb"
"0x0"
"hastebin"
"termbin"
This resolves #1416.
Example
Potential problems
It seems that if I use
.help
, the bot will cache the url. If I then change theoutput
option to something else, the bot won't let me use the new service since it still has the old link cached. This could lead to problems if the old link is erroneous and the user can't get a new one until they restart the bot.@dgw Is it possible to make it so that changing the config option also triggers the cached data to be cleared? I think that would be the simplest solution.
Next steps?
Version