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

Refactor for v3 #10

Merged
merged 7 commits into from
Nov 6, 2023
Merged

Refactor for v3 #10

merged 7 commits into from
Nov 6, 2023

Conversation

catornot
Copy link
Member

@catornot catornot commented Jun 23, 2023

refactor for v3 with fixes :)

also should this repo contain a plugin to load v2 plugins on v3 or should it be a separate thing?

@GeckoEidechse
Copy link
Member

also should this repo contain a plugin to load v2 plugins on v3 or should it be a separate thing?

uh wdym? This repo is only be DiscordRPC stuff. I would just release that as a separate plugin. Could maybe add it to tools org if you wanna ig?

@catornot
Copy link
Member Author

catornot commented Jun 24, 2023

uh wdym? This repo is only be DiscordRPC stuff. I would just release that as a separate plugin. Could maybe add it to tools org if you wanna ig?

just asking if it should be included with n*

@GeckoEidechse
Copy link
Member

just asking if it should be included with n*

Given that we don't have many plugins in general yet (outside of yours :P) I don't really see the need to. Trying to maintain too much backwards compatibility introduces a lot of baggage especially longterm. So as long as there isn't a specific need for it, I wouldn't include it with Northstar ^^

Copy link

@uniboi uniboi left a comment

Choose a reason for hiding this comment

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

Code looks good, however I cannot test as I don't have a discord client installed

Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Just a quick first pass for now. Will do a more in depth review once current stuff is resolved.

Also a few more doc comments for newly added functions would be nice ^^

src/utils.rs Outdated Show resolved Hide resolved
src/utils.rs Outdated Show resolved Hide resolved
src/presense.rs Outdated Show resolved Hide resolved
@catornot
Copy link
Member Author

Just a note for anyone merging this; rrplug for v3 hasn't been published to crates io yet since I can't quite decide on some major things. Also rrplug should pinned to a specific commit hash or to version in the future.

Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Confirmed working in testing together with R2Northstar/NorthstarLauncher#472 and R2Northstar/NorthstarMods#652

Joined a bunch of servers and watched Discord activity status update

(Still wanna do some code review though)

@@ -6,9 +6,10 @@ edition = "2021"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
rrplug = { version = "2.0.0", features = ["presence"] }
rrplug = { git = "https://github.com/R2NorthstarTools/rrplug" }
Copy link
Member

Choose a reason for hiding this comment

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

What's the current state of publishing to crates.io?

I'm fine with merging while still pointing to git repo but I'd kinda wanna see rrplug for v3 being on crates.io soon after so that it's more accessible to anyone developing plugins ^^

Copy link
Member Author

@catornot catornot Nov 6, 2023

Choose a reason for hiding this comment

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

I can publish it at any moment since it's basically done, but I would rather not.
The main is issue is that I don't quite know how much traits/generics I need over macros.

currently it's mix of both. I want to remove as much macros as possible. Due to how rrplug has to interact with the plugin system and the engine for some cases it's hard to decide what would be better.

src/discord.rs Show resolved Hide resolved
src/discord.rs Show resolved Hide resolved
@GeckoEidechse GeckoEidechse merged commit 143dada into R2Northstar:main Nov 6, 2023
1 check passed
@catornot catornot mentioned this pull request Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants