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

[Audio] Playlist rewrite #2861

Closed
wants to merge 42 commits into from
Closed

[Audio] Playlist rewrite #2861

wants to merge 42 commits into from

Conversation

Drapersniper
Copy link
Contributor

@Drapersniper Drapersniper commented Jul 13, 2019

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

  • Bug Fixes
  1. Playlist save fix (will overwrite playlists with no confirmation)
    • Playlist are now stored by ID (ctx.message.id), this allows multiple playlist with the same name
  2. [p]playlist remove now converts a playlist to a custom one if it was created from a link
    • Before this part of the code was always ignored
  • Done
  1. Moved Playlist to a custom scope, there are 3 new custom scopes
    • Global Which only Bot Owner have edit access
    • Guild Which bot owners, DJ, Mods, Admins, Guild Owners, Authors have access to
    • User which bot owner and playlist author have access
  2. Playlists are now stored in Playlist object
  3. Created API's to easily access/modify/delete Playlists based on the ModLog structure
  4. Wrote a function that convers the old config schema to the new versions with the
  5. Update all playlist commands to use the new Playlist objects
  6. Rename Playlists command [p]playlist rename added
  7. V3 now have their own playlist schema that has song information, so when a playlist is uploaded no song information will need to be refetched, keeping support for V2 playlists
  8. Making a dynamic converter for the Playlist ID, so that users can give the bot either the Playlist ID or the playlist name, it will prompt user to select one if multiple matches are found
  9. Added [p]playlist update (If a playlist has a playlist url saved (non-custom playlist)) will now update the playlist to the latest tracks avaliable
  10. Check Permissions are correct before Sending PR
  11. Update Docstrings, variable names and user help message to reflect changes
  • Will be working on

  • Make playlist remember where it was stopped #1544 Will wait for @aikaterna to finish her implementation so that this can be done with the ranking system

@Drapersniper Drapersniper mentioned this pull request Jul 13, 2019
26 tasks
@Drapersniper Drapersniper changed the title [Audio] Playlist rework [Audio] Playlist rewrite Jul 13, 2019
Allow single word playlist names
Playlist made in V3 with the new schema will have the Playlist ID as the file name when downloading them
@Drapersniper
Copy link
Contributor Author

@Drapersniper Drapersniper commented Jul 14, 2019

@aikaterna, I was thinking so improve user help message and functionality and accurary

a converter that parses scope,author,guild all in one go, it would essentially make the commands more complex to run it would be like [p]set apikey but for a normal user this wouldn't matter as they would be able to use everything as they did prior to update

… new arguments added by this PR

NOTE: From the user perspective nothing changes...
The argparser is only activated then the user tried access the new feature otherwise it stays exactly the same as it is in 3.1.3

Signed-off-by: Guy <guyreis96@gmail.com>
Signed-off-by: Guy <guyreis96@gmail.com>
Signed-off-by: Drapersniper <guyreis96@gmail.com>
@Drapersniper
Copy link
Contributor Author

@Drapersniper Drapersniper commented Jul 15, 2019

Okay, so I had a brief chat with @mikeshardmind about an argparse for these new commands and how it's unlikely to be implemented into core Red without a full review/discussion due to the complexity added.

So wanted to get the conversation started to reach the best possible outcome for these changes.

I propose that we allow argparser for this PR.
Note: The average user would never need to use these as if not provided it will always default to the current Audio behavior seen in 3.1.3 and before for Playlists. However if the user wishes to use a non default scope/settings for playlist this will be when the arpsarser arguments would come in play.

Arguments

  • --scope <user|global|guild>
  • --author [id|name]=ctx.author
  • --guild [id|name]=ctx.guild

Benefits

  1. This will allow [p]help playlist to stay simple where the breakdown of the argparser is done in the additional info
  2. This will allow more accurate commands than before as before I was using Optional[]=None, However this could cause problem (If a user/server name is the same name as a playlist as an example)
  3. This will easily allow users to access different scopes, such as get info/play another users playlist in the USER or GLOBAL scope with a simple argument --scope user|global
  4. If user doesn't pass an argparse argument to the command it will always default to ctx.author,ctx.guild, GUILD scope
  5. If --author or --guild are invalid it defaults to ctx.author,ctx.guild,
  6. Would allow Input with space in user names and guild names if they don't have/user the ID

Less positive implications

  1. This adds extra complexity to the playlist commands
  2. This can be confusing to unfamiliar users if not documented properly
  3. This is a non-standard parsing when compared to the rest of Core

Given the benefits and how your everyday user will not need to use it due to defaults I think this change would be beneficial as it improves usability for advanced users who wish to use the multi scope functionally that this PR adds

@Drapersniper
Copy link
Contributor Author

@Drapersniper Drapersniper commented Jul 15, 2019

@jack1142 Suggested splitting up the commands into groups but this can be cumbersome and make it more difficult to maintain

It's a great Idea if we can figure out a way to make groups take arguments

Just wanted to put it in the radar of anyone viewing/reviewing this PR.

Signed-off-by: Drapersniper <guyreis96@gmail.com>
[p]playlist update added

Signed-off-by: Drapersniper <guyreis96@gmail.com>
Signed-off-by: Drapersniper <guyreis96@gmail.com>
Signed-off-by: Drapersniper <guyreis96@gmail.com>
Signed-off-by: Drapersniper <guyreis96@gmail.com>
LazyGreedy converter

Signed-off-by: Drapersniper <guyreis96@gmail.com>
Fixed Dynamic Converter
Added argparser arguments to help

Fixes a few typos killed a few unicorns added a few TODO and FIX ME

Signed-off-by: Guy <guyreis96@gmail.com>
@jack1142
Copy link
Member

@jack1142 jack1142 commented Jul 15, 2019

Yeah, so while there was a chance to make groups take arguments (not ideal but I think we could work with it), I gave up on this when it turned out that subcommand's help doesn't show group's args, which makes this pointless. There's just no good way to get help to show group's syntax in a subcommand's help.
So, while I would love to not add argparser, especially for commands that are gonna be used by regular users, I just can't think of a way to do it differently.

Signed-off-by: Drapersniper <guyreis96@gmail.com>
Signed-off-by: Drapersniper <guyreis96@gmail.com>
…ugging

Signed-off-by: Drapersniper <guyreis96@gmail.com>
Reformatted Playlist Picker to be multiline to support mobiles better
More TODOs
Made Playlist converter a partial match

Signed-off-by: Drapersniper <guyreis96@gmail.com>
…s auto handled by `on_command_error`

Signed-off-by: Guy <guyreis96@gmail.com>
Signed-off-by: Draper <guyreis96@gmail.com>
Signed-off-by: Draper <guyreis96@gmail.com>
Signed-off-by: Draper <guyreis96@gmail.com>
Signed-off-by: Draper <guyreis96@gmail.com>
Signed-off-by: Draper <guyreis96@gmail.com>
@Drapersniper
Copy link
Contributor Author

@Drapersniper Drapersniper commented Jul 17, 2019

Marking this as Ready for review, however I may or may not do some tweaks to this one to reflect @aikaterna 's PRs If those get released before this one if fully reviewed.

Given the complexity of argparser I attempted to give very clear and direct errors messages if the user tried to use argparser as part of the commands. (Note: All user facing commands do not require any arg parsing unless they wish to change scopes, guild, author. The Only exception of this is [p]playlist copy which requires argparse for it to work due to the complexity of multiple scopes)

This is what this PR looks like when a user tried to user the new arguments incorrectly.

Context help will have a full break down of the command.

image

If the user gives an invalid input to the commands:

image

If the user doesn't use argparser then the default behaviour is the same as the behaviour prior to this being merged

image

@Drapersniper Drapersniper marked this pull request as ready for review Jul 17, 2019
Signed-off-by: Draper <guyreis96@gmail.com>
Signed-off-by: Draper <guyreis96@gmail.com>
Fixed some some strings to make it more user friendly

Signed-off-by: Draper <guyreis96@gmail.com>
Signed-off-by: Draper <guyreis96@gmail.com>
@Drapersniper
Copy link
Contributor Author

@Drapersniper Drapersniper commented Aug 1, 2019

Superseded by #2904

@Drapersniper Drapersniper deleted the audo_config branch Aug 1, 2019
@jack1142 jack1142 added the Type: Bug label Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Type: Enhancement Type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants