NOVA-Team / NOVA-Monorepo

The core API of the NOVA voxel game modding system
https://nova-team.github.io
GNU Lesser General Public License v3.0
66 stars 23 forks source link

[RFC?] Redesign commands #224

Open SoniEx2 opened 8 years ago

SoniEx2 commented 8 years ago

Current idea:

Args ->
  get(int index) ->
    has();
    as<primitive type>(); (e.g. asInt(); asLong(); etc)
    as(Class<? extends ArgConverter<T>> clazz);
  asList();

Questions:

  1. Should we have asString?
  2. Should as take any Class, and if there's no converter for it then cause a runtime error? (This seems like bad API design, but it'd let you pass in String.class)
  3. Should Args.asList() return the backing list, or should it return a copy of the backing list? Should we wrap the backing list such that it doesn't accept null?
  4. Should we implement Collection?
RX14 commented 8 years ago

What's the use of the has() function?

SoniEx2 commented 8 years ago

Instead of returning a proper List, we'd return a wrapped List that returns an empty Arg for args out of bounds.

RX14 commented 8 years ago

That appears to be a really bad idea to me.

SoniEx2 commented 8 years ago

Could always use Map<Integer, Arg> instead.

RX14 commented 8 years ago

Could always expect programmers to be able to use List#size...

SoniEx2 commented 8 years ago

Returning a valid value on out of bounds without changing List.size() should be safe. Not sure if it's allowed by the List.get contract but w/e, it doesn't matter, your code shouldn't rely on OutOfBoundsExceptions anyway.

RX14 commented 8 years ago

No, this clearly breaks the contract of List<T>. Nobody expects List to do that. If you want to create an interface like this, then we should create an Args class instead of List<Arg>.

SoniEx2 commented 8 years ago

With Args.asList()?

RX14 commented 8 years ago

Maybe, but not as the main API, it should have size() and get(int) (like List<T>), but return EmptyArg classes instead of index error.

SoniEx2 commented 8 years ago

Should Args.asList() return a List<Arg> view of the args? Or should it return a List<Arg> copy of the args?

RX14 commented 8 years ago

@SoniEx2 semantics, return a Collections.immutableList view of it.

SoniEx2 commented 8 years ago

@RX14 why?

RX14 commented 8 years ago

Why not?

SoniEx2 commented 8 years ago

@RX14 Well idk but why should Args be immutable?

RX14 commented 8 years ago

Because they are in reality immutable, you run the command with arguments, and you can't retroactively change what the user meant.

SoniEx2 commented 8 years ago

@RX14 Technically, you're given an Args. The Args is never used anywhere else so you're free to do with it as you like.

RX14 commented 8 years ago

Technically, you're wrong.

SoniEx2 commented 8 years ago

@RX14 Why limit the user?

RX14 commented 8 years ago

because it's the true state of the world that you cannot retroactively change the arguments the user passed into a command. That's time travel and pretending otherwise has no benefits and doesn't show the true state of the user interaction.

SoniEx2 commented 8 years ago

@RX14 Everything is mutable if you put enough hacks into it. And you might want to pass the Args forward after some tweaking.

RX14 commented 8 years ago

@SoniEx2 I have explaied why multiple times, and I'm pretty sure the other developers will agree with me that having Args be mutable "feels wrong". You can construct your own Args objects with your own list.

calclavia commented 8 years ago

I agree with having Args be immutable.

ExE-Boss commented 7 years ago

Should there be a CommandFactory or should it just be a Command instance?

SoniEx2 commented 7 years ago

I'm not sure the purpose of a CommandFactory other than to waste my RAM?