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

Add NodeInjectionService (#153, solution 2) #233

Closed

Conversation

VelvetToroyashi
Copy link
Contributor

@VelvetToroyashi VelvetToroyashi commented Sep 13, 2022

This PR implements support for solution 2 mentioned in #153.

I've yet to add tests (should be fairly simple). Solution 1 may have been easier considering interactivity being rewritten to piggyback off of Remora.Commands, but I already had the code written, and hadn't pushed.

If solution one is preferred, 90% of the code can be reused so feel free to poke about that. cc @Nihlus

( Fixes #153 )

@Nihlus
Copy link
Member

Nihlus commented Sep 26, 2022

I'm kind of still in the solution 3 camp, myself. That said, this is not orthogonal to that and could be merged as a stopgap measure.

@VelvetToroyashi
Copy link
Contributor Author

The third option is probably the best? Certainly a fair bit more work to implement, so I second this being a stopgap.

@Nihlus
Copy link
Member

Nihlus commented Oct 8, 2022

You've got some xUnit warnings to take care of.

@Nihlus
Copy link
Member

Nihlus commented Nov 22, 2022

Closing as moot.

@Nihlus Nihlus closed this Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: Future of command contexts and making contextual information more verbose
2 participants