[V3 General] Changed urban dictionary for my embed version #1871
Conversation
Tobotimus
changed the title
|
I'll update my branch (to include the changes made to V3/develop) after the code is reviewed if you don't mind. I don't really want to update this branch all the time while I wait for review, hope you understand. |
There was a problem hiding this comment.
Hi, thanks for your contribution! 🎉
I like the new menu interface, much better than the old one! However there are a couple things which need to be changed in your code before we'll accept this 😃
Once you've made the changes, please post a comment to let us know.
redbot/cogs/general/general.py
Outdated
| async def urban(self, ctx, *, search_terms: str, definition_number: int = 1): | ||
| """Urban Dictionary search | ||
| async def urban(self, ctx, *, word): | ||
| """Searches for urban dictionary entries using the unofficial urbandictionary api""" |
There was a problem hiding this comment.
This help string is a bit long. Try to make the first line shorter than 72 characters, but the user should still know exactly what the command does from that line. Any further detail should be expanded after two line-breaks.
redbot/cogs/general/general.py
Outdated
|
|
||
| Definition number must be between 1 and 10""" | ||
| data = None |
There was a problem hiding this comment.
Early declaration of variable here is unnecessary
redbot/cogs/general/general.py
Outdated
| except: | ||
| await ctx.send(_("Error.")) | ||
| await ctx.send("No Urban dictionary were found or there was an error in the process") |
There was a problem hiding this comment.
Also, this sentence isn't quite grammatically correct, and needs to be wrapped in _()
redbot/cogs/general/general.py
Outdated
| embed.url = ud["permalink"] | ||
| embed.description = ( | ||
| ud["definition"] + "\n \n **Example : **" + ud.get("example", "N/A") | ||
| ) |
There was a problem hiding this comment.
Please use "".format() instead of concatenating strings, and also add back in the i18n call (which is surrounding the string with _()).
redbot/cogs/general/general.py
Outdated
| + " Down / " | ||
| + str(ud["thumbs_up"]) | ||
| + " Up , Powered by urban dictionary" | ||
| ) |
There was a problem hiding this comment.
Please use "".format() here too
|
I think I've changed everything you asked for. Thanks for the review. :) Could it be in Beta 17 or is it feature-freezed? 🤔 |
|
Had to come up with a fix for the 2048 character limit of descriptions, I hadn't thought of it. Everything should be good now though. |
|
Just commenting again since this is supposed to be for B18 according to the tag so I'd like a review again. Thank you. |
|
I'm going to place this on hold to review with other core developers. See what they think. |
|
This is sleek, and I like it a lot. The only problem I have with it is that it does not respect the core embed requested setting. It might be better to break this out into 2 separate functions, (old code and new) and have the appropriate one based on that setting called when the command is invoked. |
|
I don't know how to respect the embed setting, if you could give me pointers, I'd be glad to do it, @mikeshardmind. Feel free to PM me in Discord instead of commenting here so we can do this faster! 😄 |
|
There's a helper method in context for this since this particular command always should respond in the same location it was called from. if await ctx.embed_requested():
# output version using an embed
else:
# output version without an embed |
|
I think I did it properly so this should be done now! (sorry for the wait, I was a bit busy) |
|
Looks solid. 👍 |
There was a problem hiding this comment.
Alright, thanks @lionirdeadman for the PR and thanks @mikeshardmind for reviewing. This has really jazzed up the urban command.
A small word of advice @lionirdeadman for any future PR's, try not to change too much code unnecessarily. This command has essentially been rewritten from the ground up when looking at the diff - but in retrospect it only aimed to change the output format (not any HTTP request / backend stuff). Changing more than necessary gives the reviewers more work than necessary, hence why this has taken almost 2 months to be merged! However I'll let it slide and I hope you're glad this has finally had its green button pressed :)
|
I had already made that code in a cog, I simply modified it slightly for this PR. I'll try not to do that again though if that makes things harder for you guys. And yeah, I'm pretty happy it got merged. ^^ |
Type
Description of the changes
Replaced the old urban dictionary command to an embed version I made which adds: