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

NOVA-Commands Implementation #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

NOVA-Commands Implementation #1

wants to merge 3 commits into from

Conversation

ExE-Boss
Copy link
Member

@ExE-Boss ExE-Boss commented Jan 10, 2017

This is my draft of NOVA-Commands.
I’ve implemented this according to the discussion at NOVA-Team/NOVA-Monorepo#224.

It might be best to wait until NOVA-Team/NOVA-Monorepo#244 is resolved, so that I can optionally add JSON to Data deserialization support.

To do:

  • Fix the ArgsParser class to be correct according the POSIX Utility Arguments Syntax.
  • Handle character escaping in an utility that will be utilised by the wrappers so that the command that gets passed to NOVA mods is already parsed properly.

Closes NOVA-Team/NOVA-Monorepo#224
Note that as the creator of this repository, I can merge this PR myself.

@ExE-Boss ExE-Boss self-assigned this Jan 10, 2017
@Victorious3
Copy link

Victorious3 commented Jan 10, 2017

Minecraft's arguments are for commands are pretty basic, I'd support some version of named arguments at least, like in the form of GNU's getopt:

command arg1 arg2 --longopt=5 -s

The natural representation of that would be a hash map. Arguments could be handled by an argument parser that verifies the supplied arguments, and parses them once and not on every access. Other games could potentially use other formats, so they could supply their own version of an argument parser.

@Victorious3 Victorious3 reopened this Jan 10, 2017
@ExE-Boss
Copy link
Member Author

I’ve implemented the first pass of arguments parsing.
Now, all that’s left is a GNU-like arguments parser and have commands supply the parser they want.

@Victorious3
Copy link

Victorious3 commented Jan 11, 2017

Wow that's one complex approach.

I have a simpler one, store arguments by their respective types and parse them in one pass. Imagine something like this:

void callback(ArgParser p) {
    p = p.args(int.class, float.class, String.class).opt(int.class, "example", "e"); // And so on
    int arg1 = p.get(0);
    // ...
    int example = p.getOrElse("example", 5);
}

ArgsParser would be immutable so that you can have multiple sets of arguments for a command.

I haven't quite thought this out yet completely but it's roughly inspired by python's implementation of gnu style options. It spares you the need for creating countless of wrapper types.

@Victorious3
Copy link

Has my thumbs up

@RX14
Copy link

RX14 commented Jan 12, 2017

@Victorious3 can you do a proper github review next time?

@Victorious3
Copy link

Fancy new feature I won't try to explore with my phone, sorry :P

@RX14
Copy link

RX14 commented Jan 12, 2017

@Victorious3 it works fine on the phone, just as you expect...

@Victorious3
Copy link

@RX14 I'm not expecting much considering that I cant even edit my comments without requesting the desktop version.

@RX14
Copy link

RX14 commented Jan 12, 2017

@Victorious3 well it does work so ok

@ExE-Boss ExE-Boss force-pushed the master branch 3 times, most recently from dcae3b5 to bef0988 Compare January 13, 2017 19:56
@ExE-Boss ExE-Boss force-pushed the implementation branch 8 times, most recently from dd94c58 to a5109ac Compare January 13, 2017 22:22
@ExE-Boss ExE-Boss changed the title NOVA-Commands implementation NOVA-Commands Implementation Jan 13, 2017
@RX14 RX14 requested a review from Victorious3 January 13, 2017 22:23
EnumSelector<Errors> errored = EnumSelector.of(Errors.class).blockAll();

for (String stringArg : args) {
if (optional || continuos) {
Copy link

Choose a reason for hiding this comment

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

continuos is a typo

public void testEquals() {
assertThat(args0.equals(args0)).isTrue();
assertThat(args2.equals(args3)).isFalse();
assertThat(args1.equals(new ArgsParser("Arg1").args(String.class).parse())).isTrue();
Copy link

Choose a reason for hiding this comment

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

isEqualTo assertion if possible?


@Test
public void testHashCode() {
System.out.println("args0: " + args0.hashCode());
Copy link

@RX14 RX14 Jan 13, 2017

Choose a reason for hiding this comment

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

No printlns in test code! The results must be verifiable in code. This is useful for getting started but should be codified later.

@RX14
Copy link

RX14 commented Jan 13, 2017

@Victorious3 do you want to review this?

@ExE-Boss ExE-Boss force-pushed the implementation branch 2 times, most recently from cf06423 to d9d9bd9 Compare January 14, 2017 19:40
@ExE-Boss ExE-Boss force-pushed the implementation branch 2 times, most recently from 306fdc5 to 00bd056 Compare February 3, 2017 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC?] Redesign commands
3 participants