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

Nim for Discox #188

Closed
wants to merge 1 commit into from
Closed

Nim for Discox #188

wants to merge 1 commit into from

Conversation

xTrayambak
Copy link

What kind of change does this PR introduce?
This PR allows you to write commands for Discox within the Nim programming language using the dimscord library.

Describe the changes proposed in the pull request
The Python code is now run by an init binary written in Nim, meaning everything written in Python still works just as it would.

Does this PR introduce a breaking change?
Nothing. The makefiles and Nix flake have been modified to work with the new system from now on.
Note that nim and nimble are now a required dependency to run discox.

Does this PR introduce changes to the database?
No.

Other information:
Nim is very based.

please accept this, I spent 3 hours of my life doing this
kthxbye :3

@github-actions github-actions bot added the Not Reviewed This pull request is pending review label Oct 31, 2023
@timelessnesses
Copy link
Contributor

jesus christ

Copy link
Contributor

@timelessnesses timelessnesses left a comment

Choose a reason for hiding this comment

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

do we have something like python bindings instead of spawning 2 instances of bot?, still no support for mysql bridge

bot/bridge/bot.nim Show resolved Hide resolved
Comment on lines +40 to +59
EmbedField(
name: "**URL:**",
value: "```" & VirboxMcServer & ':' & VirboxMcServerPort & "```"
),
EmbedField(
name: "**VERSION:**",
value: "```" & server.version() & "```"
),
EmbedField(
name: "**PLAYERS:**",
value: "```" & playerHeader & "```"
),
EmbedField(
name: "**PLUGINS:**",
value: "```" & pluginHeader & "```"
),
EmbedField(
name: "**PROTOCOL**",
value: '`' & $server.protocol().get() & '`'
)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not identical to the python version

Copy link
Author

Choose a reason for hiding this comment

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

There's a few extra fields, but other than that it looks the same. I'll fix the embed color.

@xTrayambak
Copy link
Author

jesus christ

🧌

@jtbx
Copy link
Contributor

jtbx commented Oct 31, 2023

No.

@xTrayambak
Copy link
Author

Why not? Doesn't hurt anyone, plus I'll be able to contribute to more commands since I've basically forgotten Python.

@jtbx
Copy link
Contributor

jtbx commented Oct 31, 2023

I think this is a bad idea because you've redesigned the core bot "infrastructure" to use Nim primarily; no one will be able to maintain it except you.

@xTrayambak
Copy link
Author

It's very easy to change it, even now. It seems you haven't seen the source code and are assuming things.

@jtbx
Copy link
Contributor

jtbx commented Oct 31, 2023

I have seen your changes but I just think it reduces maintainability moving parts of it to Nim. Nobody even knows how to use Nim's tooling except you, nor does anyone here use the Nim language, so why is this even needed?

The Python interpreter is a common program found on most systems but Nim is not, further increasing the need for unnecessary dependencies.

@xTrayambak
Copy link
Author

Nim's tooling is something a literal child can use. Plus, you don't even need to. I've fixed the Makefile and Nix Flake to work properly, so in the best case, you won't have to. I'll take care of any issues that occur, though there should be none.

@xTrayambak
Copy link
Author

Nim is available in a lot of software repos nowadays, with the major exception being Fedora Linux.

@timelessnesses
Copy link
Contributor

i think this can be improved to something like
a program that is built to an executable kinda like

you have 2 commands one for listing commands with its description and one for calling the command. then there's wrapper for each programming language that communicate through stdin and stdout in something like json format. then it could in theory support every other languages

@timelessnesses
Copy link
Contributor

so now mysql bridging isn't the problem because python side would query it then send it back through stdin

@timelessnesses
Copy link
Contributor

let me come up with a binding rq

@gagero
Copy link

gagero commented Nov 1, 2023

@xTrayambak Let me explain to you so you'll understand: No one cares how "easy it is to use Nim's tooling" or how "it's in every repo." No one but you cares about Nim. More importantly, no one but you knows Nim. You have rewritten core parts of the bot in Nim, meaning that no one but you can maintain it because I guarantee, no one will learn Nim for a Discord bot. Your PR would provide absolutely no benefit but complicate both the bot itself and the infrastructure needed to operate the bot. Furthermore, it will make the bot unmaintainable. Making core parts of the software in Nim is the exact opposite of making it optional.

@xTrayambak
Copy link
Author

i think this can be improved to something like
a program that is built to an executable kinda like

you have 2 commands one for listing commands with its description and one for calling the command. then there's wrapper for each programming language that communicate through stdin and stdout in something like json format. then it could in theory support every other languages

This seems like a good idea. I'll try to implement this.

@jtbx
Copy link
Contributor

jtbx commented Jan 24, 2024

What's the current status on this?

@xTrayambak
Copy link
Author

Turns out, I never got around to working on that

@xTrayambak xTrayambak closed this by deleting the head repository Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not Reviewed This pull request is pending review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants