Modify command implementation to use a Command interface
Opened this issue · 8 comments
Currently, commands are being defined with functions that typically end up calling processCmdToEvent(Arr). As the functions are private methods, there are no options for commands to be defined for mods and datapacks in other packages while still using this package. A fork would be required.
Consider instead of defining a function for each command, define a struct fro each command that satisfies a Command interface which is passes to a Command function, which handles stringing the command to pass to the console, and reading/handling the events associated with that command. For example:
type Command interface {
Command() string
Events() []events.Event
}
type ExperienceAdd struct {
Target string
XP int32
XPType ExperienceType
}
func (c ExperienceAdd) Command() string {
return fmt.Sprintf(experience add %s &d %s, c.target, c.xp, c.xpType)
}
func (c ExperienceAdd) Events() []events.Event {
return []events.Event{
events.ExperienceAdd,
events.NoPlayerFound.
}
}
func (w *Wrapper) ExecuteCommand(c Command) error {
ev, err := w.processCmdToEvent(c.Command(), time.Second, c.Events()...)
if err != nil {
return err
}
if ev.Is(events.NoPlayerFoundEvent) {
return ErrPlayerNotFound
}
return nil
}Please note this is not a real implementation, I just adapted the ExperienceAdd function to what it could look like in this Command interface format.
If this is an idea you are open to, I would be happy to work with you on a full implementation before switching from the current function approach.
@densestvoid thats perfect! I like it, feel free to drop a pr. We can retrofit the current funcs to use the Command afterwards.
My use case was simpler than yours, can you provide an example Command that you had to define yourself? I'm trying to understand the full extend of it.
Why I ask is because, not all log lines are processed to events rn and exposing the Command interface but they don't have control over adding Events, if they need to parse a specific Event that does not exist yet, they will need to send a PR here to add it (as of right now). Hence of original thought of just calling a funcs with your set of args, and not having to deal with the logs/events. If you don't think that would be an issue, then I'm all good for it 👍
I don't have an example per say. I was just looking down the road. The two things I could see being constant PR requests are:
- Mod/Datapack support
- Old version support
Both of which are addressed by creating an interface that anyone can define a command on. Specifically, this project could have a commands subfolder for each version of commands, so you import the version you are targeting.
Events could be handled the same way, defining an interface, which would parse itself.
Not saying any of this is easy, I haven't looked in depth at Event parsing, but something to consider. I will whip up a bare bones PR with a couple commands so we can flesh out the idea before moving to rewriting all commands this way.
Yea, definitely didn't plan on supporting across versions. Sounds like a good plan to me. Looking forward to the PR, thanks.
Concerning events, the Event is already an interface and I had plan to re-work it as well. Might be a good opportunity to include a sort of registerGameEvents based on the commands being called (ie parse only events/regexes that we expect to be useful). Since it will get real expensive to loop thru each regexes for each lines (https://github.com/wlwanpan/minecraft-wrapper/blob/master/logparser.go#L46) as the list grows :S
I am still working on the PR, but wanted to ask if there is a way to parse multiple lines for events, as most error events have a multi line output.
I have uploaded my changes to my fork. I haven't submitted it as a PR because I haven't reworked the processing commands functions yet, as I want to confirm that you like this approach so far before moving forward.
I noticed that you are registering channels for events during command processing, and it reminded me of a package I started writing not too long ago. The package acts as a channel handler between senders and receivers, which I think could be applied to how the event channels are being handled here. Let me know what you think.
I am still working on the PR, but wanted to ask if there is a way to parse multiple lines for events, as most error events have a multi line output.
@densestvoid you can take a look at processCmdToEventArr used in the BanList which reads multiple log lines and aggregate them.
I noticed that you are registering channels for events during command processing, and it reminded me of a package I started writing not too long ago. The package acts as a channel handler between senders and receivers, which I think could be applied to how the event channels are being handled here. Let me know what you think.
Concerning the postoffice package, I'm a bit mindful when it come to importing new dependencies :) We'll revisit if our use case changes, as it works as expected for now.
I haven't submitted it as a PR because I haven't reworked the processing commands functions yet, as I want to confirm that you like this approach so far before moving forward.
Sorry for the delay, for the approach looks good to me, left some comments on the PR you sent.
No worries about the postoffice package. I may have been over excited, as it was just an idea I had been working on, and thought it might fit well here. I understanding not wanting to introduce new dependencies.
I have been quite distracted recently, I apologize. I will try to have a prototype to you by the end of March.