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 commands #46

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

md5sha256
Copy link
Member

This PR basically overhauls the original HyperCommandManager and splits the code into two classes - HyperCommand and HyperCommandUtil. The naming schemes are not exactly ideal. The HyperCommandUtil is essentially a singleton and deals with registering contexts, completions and locale messages. I am aware it does not look like a proper singleton at this point and this should be fixed before merging.

@md5sha256
Copy link
Member Author

The singleton thing should now be resolved. I guess naming schemes and the long methods are left to fix. Would it be worthwhile splitting commands of each category into their own (respective) classes?

@Citymonstret
Copy link
Member

I think we should split all context & tab completion handlers into their own (enum) singleton classes tbh, would do a lot for readability

@md5sha256
Copy link
Member Author

Right, but say we split the commands into their own classes, should we register command contexts & completions by each command group, or everything in one place (or two, as per your suggestion)?

@Citymonstret
Copy link
Member

I think we can keep the command methods in one class, but we should really be giving each context resolver its own class (because they're just interfaces)

Although I wouldn't mind splitting command categories up either, like WorldEdit does it 🤷

@md5sha256
Copy link
Member Author

Alright, I'll implement then mark dis as ready to review.

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.

2 participants