Skip to content
This repository has been archived by the owner on May 1, 2022. It is now read-only.

[FEAT] Merge with dis-snek #3

Open
11 of 13 tasks
AstreaTSS opened this issue Mar 15, 2022 · 11 comments
Open
11 of 13 tasks

[FEAT] Merge with dis-snek #3

AstreaTSS opened this issue Mar 15, 2022 · 11 comments
Assignees
Labels
help wanted Extra attention is needed

Comments

@AstreaTSS
Copy link
Contributor

AstreaTSS commented Mar 15, 2022

This was the end goal, after all. Fixes dis-snek's #392.

Merging molter is not an easy task. Ideally, it would be done before the beta soft deadline, but if this is not complete, this should not hold dis-snek back from going into beta.

In terms of molter itself, there's a few things that need to be finished up:

  • Bug fixing, obviously. Not many people have used the advanced features, so that's something to consider.
  • Making a help command. Technically, this isn't a strict requirement, but being honest, people who use message commands will want one - it's worth providing one under the ext namespace.
  • Helper functions and properties, largely for:
    • Allowing custom usage specifications instead of forcing people to use signature.
    • Speaking of which, refactoring signature if possible.
    • Probably more I can't think of quite yet. For Github's sake, this is going to be marked as done, but it might not be.
  • At least finish the docstrings. Doing a whole page would be nice.
  • Subclassing BadArgument to have more specific errors. Is not strictly necessary to merge and may be ignored, but I'd prefer this being done.
  • Make invoked_name way better. It's, uh, a mess right now.
  • Add command to Context in dis-snek itself.

In terms of merging molter itself... yeah, that's going to be a doozy.

  • A guide about how to use message commands would be a requirement at this point.
  • Adjust Converter to either be not message command specific, or rename it to indicate it being so.
  • File restructuring, separating message commands into its own file at the very least. I don't actually want to touch the BaseCommand too much.

If anyone sees this and wants to help out, go ahead!

@AstreaTSS
Copy link
Contributor Author

Note: Work for the merge is being done in the merge branch. The main and dev branches will continue following the master and dev branches dis-snek has, but will not receive updates for the near future except to keep up with any breaking changes upstream.

This was referenced Apr 5, 2022
@LordOfPolls
Copy link
Member

Helper functions that[...]
Removing commands.

Surely removing commands should be handled by the lib itself? Not the merging-molter code?

@LordOfPolls
Copy link
Member

Also can you convert this into a checklist? So we know whats done and whats not

@AstreaTSS
Copy link
Contributor Author

Helper functions that[...]
Removing commands.

Surely removing commands should be handled by the lib itself? Not the merging-molter code?

There is the issue of removing subcommands (say, if a user wanted to remove a subcommand via remove_command("foo bar"), a problem that's somewhat unique to molter, but... yeah, it is. Will remove.

@AstreaTSS
Copy link
Contributor Author

Also can you convert this into a checklist? So we know whats done and whats not

Soon:tm:, but of course!

@LordOfPolls
Copy link
Member

Make invoked_name way better. It's, uh, a mess right now.

Specifically what do you want changed?

@AstreaTSS
Copy link
Contributor Author

AstreaTSS commented Apr 8, 2022

Make invoked_name way better. It's, uh, a mess right now.

Specifically what do you want changed?

I think it's easier if we look at the code behind it: https://github.com/Discord-Snake-Pit/molter/blob/f9af3325cb9a0c8c461efc4f7e93356ae92baabd/molter/overrides.py#L188-L190

...yeah, ugly, isn't it?

The problem is that invoked_name, by technicality of subcommands dpy did this, can have whitespace and newlines, while get_first_word intentionally removes/ignores them. So we have to make another way in order to get it.

(If we did just make it not have newlines... content_parameters would break if so. Or at least not function correctly. Properties are proerties, so we can't really edit it.)

The current hack is a way of doing it currently, but there has to be a better way.

@AstreaTSS
Copy link
Contributor Author

AstreaTSS commented Apr 9, 2022

Also a note: I'm thinking about splitting this into two PRs. One non-breaking change to bring the converter stuff into dis-snek with most of molter's functionality for BaseCommand, and another breaking one for just message/prefix commands.

@LordOfPolls
Copy link
Member

That's only necessary if it turns out one part isn't done. Imo it's best to just pr it all at once

@AstreaTSS
Copy link
Contributor Author

Should note that the actual merge is being actively worked on in my fork of dis-snek. There's also a thread about it on the Discord server.

@AstreaTSS
Copy link
Contributor Author

AstreaTSS commented Apr 28, 2022

The merge is basically done and awaiting review, so it might be best to mention:

After some thinking, I think I want to just archive this repo for history's sake. I feel uncomfortable using it or the PyPI package for my own experiences due to the fact that it'll stray from the original purpose of this project.

I'm fine with either moving it back to my profile to archive it or just archiving it here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants