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 links to the output of [p]repo list #6284

Open
wants to merge 12 commits into
base: V3/develop
Choose a base branch
from

Conversation

SeaswimmerTheFsh
Copy link

@SeaswimmerTheFsh SeaswimmerTheFsh commented Jan 5, 2024

Description of the changes

  • added list_urls argument to [p]repo list
    • this argument determines if urls will be output by the command
  • added codeblock argument to [p]repo list
    • this argument determines if the output of the command will be a codeblock or not. useful if you want to right click copy urls from the command's output

img

img

Have the changes in this PR been tested?

Yes

detailed changes:
- added list_urls argument
  - this argument determines if urls will be output by the command
- added codeblock argument
  - this argument determines if the output of the command will be a codeblock or not. useful if you want to right click copy urls from the command's output
@github-actions github-actions bot added the Category: Cogs - Downloader This is related to the Downloader cog. label Jan 5, 2024
@Flame442 Flame442 added the Type: Enhancement Something meant to enhance existing Red features. label Jan 5, 2024
@Jackenmen
Copy link
Member

Any reason we would want to keep the code block variant? Having two switches in a command is going to be more confusing than just one.

Another thing I wonder about is whether there's a point to a list_urls argument too, I guess it makes the list shorter when not used but do we consider this shorter variant important enough to have it?

@SeaswimmerTheFsh
Copy link
Author

SeaswimmerTheFsh commented Jan 5, 2024

I don't really know, I just didn't want to change how it already looked in a way that wasn't optional. If you'd prefer it I can remove the old codeblock entirely. I could also just make using list_urls not use a codeblock as I did initially and just keep the codeblock there if you don't set list_urls to True.

@SeaswimmerTheFsh
Copy link
Author

Do I need to do anything else for this pr to be merged @Jackenmen? Never got a response from you on my questions originally

@Jackenmen
Copy link
Member

My question was an open question to any interested party (including other maintainers of this repository). It doesn't seem like anyone cared enough to respond so I would suggest going ahead and making the [p]repo list true false (no code block, links included) variant the default one and removing the options.

@SeaswimmerTheFsh
Copy link
Author

Done!
image

@Kreusada Kreusada self-assigned this Feb 13, 2024
@SeaswimmerTheFsh
Copy link
Author

SeaswimmerTheFsh commented Mar 6, 2024

bored and thought it might be fun to add embeds to this command (using ctx.embed_requested()), would that be worth the extra code complexity

edit: probably not now that I think about it, because I don't believe red has a way to pagify embed contents built-in (if it does please let me know as this would be incredibly useful, but I haven't seen it before)

@Flame442
Copy link
Member

Flame442 commented Mar 6, 2024

The original reason for it being in a code block was to match the formatting of [p]cog list, which used the codeblock to format with diff formatting. However, it was always confusing whether green or red should mean installed vs can be installed, and the diff format color scheme became much uglier, so #6152 swapped the format to a more neutral one relying more on the headers.

The # Installed Repos was intended to be for color formatting the header, and I think it might be a bit to big if we are sticking to a codeblock-less output. ## seems like a much more reasonable size. Additionally, I'd suggest bolding the repo names to make them stand out a bit more from the short description.

it might be fun to add embeds to this command

I don't think that fits with the general style of the rest of the bot, generally Red avoids using embeds for outputs that are supposed to be more utility focused, and instead keeps them limited to mostly user focused informational commands like [p]info and [p]now.

SeaswimmerTheFsh and others added 2 commits March 6, 2024 15:45
bolded repository names and changed the size of the markdown header, see:
> The `# Installed Repos` was intended to be for color formatting the header, and I think it might be a bit to big if we are sticking to a codeblock-less output. `##` seems like a much more reasonable size. Additionally, I'd suggest bolding the repo names to make them stand out a bit more from the `short` description.
@SeaswimmerTheFsh
Copy link
Author

The # Installed Repos was intended to be for color formatting the header, and I think it might be a bit to big if we are sticking to a codeblock-less output. ## seems like a much more reasonable size. Additionally, I'd suggest bolding the repo names to make them stand out a bit more from the short description.

Done in most recent commit.

I don't think that fits with the general style of the rest of the bot, generally Red avoids using embeds for outputs that are supposed to be more utility focused, and instead keeps them limited to mostly user focused informational commands like [p]info and [p]now.

Fair enough, makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Cogs - Downloader This is related to the Downloader cog. Type: Enhancement Something meant to enhance existing Red features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants