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

Cog for code snippet links #1028

Merged
merged 39 commits into from Apr 27, 2021
Merged

Cog for code snippet links #1028

merged 39 commits into from Apr 27, 2021

Conversation

dolphingarlic
Copy link
Contributor

I thought that it would be pretty cool if Discord could show the contents of a snippet link instead of just the link to the snippet, so I made a bot for that.

However, since adding third-party bots to the Python server was out of the question, I decided to add that functionality to the official server bot instead.

Proof

@dolphingarlic dolphingarlic requested a review from a team as a code owner July 5, 2020 18:36
@dolphingarlic dolphingarlic requested review from fiskenslakt and MrHemlock and removed request for a team July 5, 2020 18:36
@ghost ghost added the needs 2 approvals label Jul 5, 2020
@ghost
Copy link

ghost commented Jul 5, 2020

Thank you for contributing to Python Discord!

Please check out the following documents:

@ks129
Copy link
Member

ks129 commented Jul 5, 2020

@dolphingarlic I don't think that this should good feature. Sending something every time when someone posts GitHub link is not a good idea. And all PRs have to have approved the linked issue, what this PR definitely don't have. Make issue first, then staff/core devs will discuss it and then decide will this come to bot or not.

@dolphingarlic
Copy link
Contributor Author

Sorry, I didn't know that I should've created an issue first. (I don't think CONTRIBUTING.md mentioned that).

I think that printing out snippets would be quite useful since it means that people wouldn't have to open the links each time anymore. Sending embeds with details about repos basically just improves what Discord already does with repo links but with more details.

@aadibajpai
Copy link

@dolphingarlic I don't think that this should good feature. Sending something every time when someone posts GitHub link is not a good idea. And all PRs have to have approved the linked issue, what this PR definitely don't have. Make issue first, then staff/core devs will discuss it and then decide will this come to bot or not.

I think this was discussed in the #meta channel over at PyDis and there was some merit in the linked idea. Personally I think it is useful, especially when getting help in any of the help channels because you either copy paste your code standalone or link to it which would mean for the other person to switch back and forth.

@sco1
Copy link
Contributor

sco1 commented Jul 6, 2020

Sorry, I didn't know that I should've created an issue first. (I don't think CONTRIBUTING.md mentioned that).

It's not mentioned because it's only a hard requirement for Seasonalbot. It's generally a good idea to make an issue first so you don't "waste" your time working on something that might not be merged, but since this seems to be something you've made for other reasons already it's a bit of a moo point.

I'm a bit meh on the repo link prettification, but the snippet formatting is deinitely interesting. We'll have to get some more opinions before we review formally, so please stand by. Is this something you've discussed in the server previously?

@dolphingarlic
Copy link
Contributor Author

Yes - I've discussed it once in #meta a while ago

@dolphingarlic
Copy link
Contributor Author

I can remove the repo link prettification if you'd like, but here's an example of when the snippet link feature is very useful:

git

Here, seeing just the single line linked in the snippet instead of having to open the link in a new tab is much more convenient. It would make debugging a lot faster.

@SebastiaanZ
Copy link
Member

SebastiaanZ commented Jul 6, 2020

As Discord already embeds a link preview by default for GitHub-links, I'm not sure if I'm in favor of having the bot respond with a second embed containing some details about the repository. Apparently this is suppressed; looked over that.

If we were to have "repository information" feature, I think it would be better as an explicitly invoked command. I'm just not sure whether or not such a command fits the scope of our community utility bot. (I really mean: "not sure"; it's not a euphemism for "I don't think it fits".)

I do like the snippet feature, although I'm a bit concerned of "surprising" vertical space that's get inserted in a conversation. Maybe we could limit it to snippets that are only a few lines long? I wouldn't be surprised if people pasting a link to a snippet of code don't expect the bot to respond with a 15-line code snippet in the middle of a conversation. Alternatively, this could also have a command interface, which ensures that it's explicitly invoked.

@Numerlor
Copy link
Contributor

Numerlor commented Jul 6, 2020

If this were to be included, I'd prefer it to be a command or limited to maybe 3 lines without directly invoking it instead of looking up links in messages. Most github permalinks that I've seen get posted in discussion channels like python-language where this behavior wouldn't be as desired
While the repo information is nice I don't think the additional info like stars is particularly useful in most situations

@ks129
Copy link
Member

ks129 commented Jul 6, 2020

One more thing: When we do this, this should get filtering too. Maybe someone wants to write a message that gets deleted by filters, but then add this GitHub and post a link. And however, this should have some text filter.

@dolphingarlic
Copy link
Contributor Author

So since nobody seems to like the repo widget prettification feature, I removed that. To address the issue of people not wanting the bot to interrupt their conversations, I added the option for the author of the message to react with an ❌ emoji to delete the bot's message.

@MarkKoz MarkKoz added a: information Related to information commands: (doc, help, information, reddit, site, tags) t: feature New feature or request p: 3 - low Low Priority labels Jul 13, 2020
bot/cogs/print_snippets.py Outdated Show resolved Hide resolved
bot/cogs/print_snippets.py Outdated Show resolved Hide resolved
bot/cogs/print_snippets.py Outdated Show resolved Hide resolved
bot/cogs/print_snippets.py Outdated Show resolved Hide resolved
bot/cogs/print_snippets.py Outdated Show resolved Hide resolved
bot/cogs/print_snippets.py Outdated Show resolved Hide resolved
bot/cogs/print_snippets.py Outdated Show resolved Hide resolved
bot/cogs/print_snippets.py Outdated Show resolved Hide resolved
bot/cogs/print_snippets.py Outdated Show resolved Hide resolved
bot/cogs/print_snippets.py Outdated Show resolved Hide resolved
@ghost ghost added s: waiting for author Waiting for author to address a review or respond to a comment and removed needs 2 approvals labels Jul 13, 2020
@ghost ghost added needs 2 approvals and removed s: waiting for author Waiting for author to address a review or respond to a comment labels Jul 30, 2020
Stuff like `/{0,1}` and `?` at the ends of groups
Copy link
Member

@MarkKoz MarkKoz left a comment

Choose a reason for hiding this comment

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

Running the code reveals errors:

https://gitlab.com/gitlab-org/gitlab/-/blob/master/scripts/create_postgres_user.sh#L3-5 leads to

2021-01-30 12:12:37 | bot.exts.info.code_snippets | ERROR | Failed to fetch code snippet from https://api.github.com/repos/gitlab-org/gitlab/branches. HTTP Status: 404. Message: 404, message='Not Found', url=URL('https://api.github.com/repos/gitlab-org/gitlab/branches').
2021-01-30 12:12:37 | bot.exts.info.code_snippets | ERROR | Failed to fetch code snippet from https://api.github.com/repos/gitlab-org/gitlab/tags. HTTP Status: 404. Message: 404, message='Not Found', url=URL('https://api.github.com/repos/gitlab-org/gitlab/tags').
2021-01-30 12:12:37 | bot | ERROR | Unhandled exception in on_message.
Traceback (most recent call last):
  File "/home/mark/repos/python/bot-pydis/.venv/lib/python3.8/site-packages/discord/client.py", line 343, in _run_event
    await coro(*args, **kwargs)
  File "/home/mark/repos/python/bot-pydis/bot/exts/info/code_snippets.py", line 228, in on_message
    message_to_send += await handler(**match.groupdict())
  File "/home/mark/repos/python/bot-pydis/bot/exts/info/code_snippets.py", line 132, in _fetch_gitlab_snippet
    refs = branches + tags
TypeError: unsupported operand type(s) for +: 'NoneType' and 'NoneType'

Looks like it's trying to use the GitHub API to fetch a GitLab file. Perhaps you need some None checks too.


Any input (except for when the above happens) will lead to

2021-01-30 12:12:32 | bot | ERROR | Unhandled exception in on_message.
Traceback (most recent call last):
  File "/home/mark/repos/python/bot-pydis/.venv/lib/python3.8/site-packages/discord/client.py", line 343, in _run_event
    await coro(*args, **kwargs)
  File "/home/mark/repos/python/bot-pydis/bot/exts/info/code_snippets.py", line 232, in on_message
    await wait_for_deletion(
TypeError: wait_for_deletion() got an unexpected keyword argument 'client'

It'd be nicer to truncate code rather than showing nothing if it's too long. Right now it isn't immediately obvious why some links don't get code blocks from the bot. However, this isn't a requirement from me and I'd approve the PR regardless.

@dolphingarlic
Copy link
Contributor Author

Truncating might be a bit tricky when there are multiple snippet links in a single message. Do you think I should do something like truncate all code blocks by ceil((len(message) - 2000) / (number of code blocks)) chars if needed? Also, what about the number of lines allowed per message?

@MarkKoz
Copy link
Member

MarkKoz commented Jan 30, 2021

I imagine that trying to evenly distribute truncation would result in all code blocks being too heavily truncated to still be meaningful. A simpler strategy of truncating the first blocks and omitting the rest sounds fine.

Line limit should still apply to the final line count and will truncate at line boundaries. The limit could be raised to 20. Make sure you don't include "bare" file paths that lack code blocks because they got truncated. In other words, if there isn't enough space for the entire file path and at least some of the code block, then omit the file path too.

I think what I'm describing is pretty similar to the extant truncation logic for the snekbox eval command. It's worth investigating how much of that code can be shared between the two features.

@Xithrius Xithrius removed the s: waiting for author Waiting for author to address a review or respond to a comment label Mar 6, 2021
Base automatically changed from master to main March 13, 2021 19:40
@Xithrius
Copy link
Member

@dolphingarlic Greetings. Will you be finishing up this PR?

@dolphingarlic
Copy link
Contributor Author

I'm still waiting for my code to be reviewed again. Sorry about the delay

@Xithrius
Copy link
Member

Thanks for the quick reply! I'll poke some people for reviews.

bot/exts/info/code_snippets.py Outdated Show resolved Hide resolved
@dolphingarlic dolphingarlic requested a review from jb3 as a code owner April 17, 2021 17:02
@Xithrius Xithrius added p: 2 - normal Normal Priority a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) and removed p: 3 - low Low Priority labels Apr 22, 2021
Copy link
Contributor

@vcokltfre vcokltfre left a comment

Choose a reason for hiding this comment

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

Looking good! Apologies I wasn't able to review this earlier I just got the notification that xith requested my review

dolphingarlic and others added 2 commits April 27, 2021 08:39
Co-authored-by: Xithrius <15021300+Xithrius@users.noreply.github.com>
@jb3 jb3 merged commit 3c0da0b into python-discord:main Apr 27, 2021
dolphingarlic added a commit to dolphingarlic/bot that referenced this pull request Apr 27, 2021
Merge pull request python-discord#1028 from dolphingarlic/master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: information Related to information commands: (doc, help, information, reddit, site, tags) p: 2 - normal Normal Priority t: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet