-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Convert mappings extension to storage unit #201
Convert mappings extension to storage unit #201
Conversation
...in/kotlin/com/kotlindiscord/kord/extensions/modules/extra/mappings/stroage/MappingsConfig.kt
Outdated
Show resolved
Hide resolved
I'm unsure what specifically to do with this one. The mappings extension is useful for sure, but it's been impossible for me to maintain it for a long time, and @sschr15 seems like he's too busy to do much with it these days. Additionally, the storage unit system needs a major rework at some point, something which I'm very unlikely to be able to do any time soon. There is a conflict to resolve here, but I'm left feeling like I didn't give you the right brief to solve this properly - but I don't have the spoons to do a proper review right now anyway, so I'm not sure. |
Hm, Ok then. I'll resolve the conflict, then let me know when you have appropriate spoons |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Fantastic that I didn't realize I had been mentioned for a bit of code I've done things with
...rc/main/kotlin/com/kotlindiscord/kord/extensions/modules/extra/mappings/MappingsExtension.kt
Outdated
Show resolved
Hide resolved
...rc/main/kotlin/com/kotlindiscord/kord/extensions/modules/extra/mappings/MappingsExtension.kt
Outdated
Show resolved
Hide resolved
It's unfortunate that this has gone so long until you managed to get a review in - but when the merge conflicts are solved, things are re-tested, and you're happy with it, we can probably merge @sschr15 |
...rc/main/kotlin/com/kotlindiscord/kord/extensions/modules/extra/mappings/MappingsExtension.kt
Outdated
Show resolved
Hide resolved
@gdude2002 Could this have a final review please? sschr15 has approved it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find much to criticise here - but as always, I need confirmation that these changes were tested.
Unless the merge commit broke something, I did testing before sending over the patch that was implemented |
That was an odd one, wasn't it? Anyway, thanks, I think we finally made it! |
Re-done this time. Should be fully functioning.