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

[RFC?] Redesign commands #224

Open
SoniEx2 opened this issue Aug 28, 2016 · 24 comments · May be fixed by NOVA-Team/NOVA-Commands#1
Open

[RFC?] Redesign commands #224

SoniEx2 opened this issue Aug 28, 2016 · 24 comments · May be fixed by NOVA-Team/NOVA-Commands#1
Assignees
Labels
breaking change This is going to break stuff in progress Pull requests that are not yet ready to be merged and issues that are being worked on. plugin task

Comments

@SoniEx2
Copy link
Contributor

SoniEx2 commented Aug 28, 2016

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?
@SoniEx2 SoniEx2 changed the title Redesign commands [RFC?] Redesign commands Aug 30, 2016
@RX14
Copy link
Contributor

RX14 commented Aug 30, 2016

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

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Aug 30, 2016

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

@RX14
Copy link
Contributor

RX14 commented Aug 30, 2016

That appears to be a really bad idea to me.

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Aug 30, 2016

Could always use Map<Integer, Arg> instead.

@RX14
Copy link
Contributor

RX14 commented Aug 30, 2016

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

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Aug 30, 2016

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
Copy link
Contributor

RX14 commented Aug 30, 2016

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
Copy link
Contributor Author

SoniEx2 commented Aug 31, 2016

With Args.asList()?

@RX14
Copy link
Contributor

RX14 commented Aug 31, 2016

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
Copy link
Contributor Author

SoniEx2 commented Aug 31, 2016

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

@RX14
Copy link
Contributor

RX14 commented Aug 31, 2016

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

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Aug 31, 2016

@RX14 why?

@RX14
Copy link
Contributor

RX14 commented Aug 31, 2016

Why not?

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Aug 31, 2016

@RX14 Well idk but why should Args be immutable?

@RX14
Copy link
Contributor

RX14 commented Aug 31, 2016

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

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Aug 31, 2016

@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
Copy link
Contributor

RX14 commented Aug 31, 2016

Technically, you're wrong.

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Aug 31, 2016

@RX14 Why limit the user?

@RX14
Copy link
Contributor

RX14 commented Aug 31, 2016

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
Copy link
Contributor Author

SoniEx2 commented Aug 31, 2016

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

@RX14
Copy link
Contributor

RX14 commented Aug 31, 2016

@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
Copy link
Member

I agree with having Args be immutable.

@ExE-Boss
Copy link
Member

ExE-Boss commented Jan 8, 2017

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

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Jan 9, 2017

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

@ExE-Boss ExE-Boss self-assigned this Jan 10, 2017
@ExE-Boss ExE-Boss added breaking change This is going to break stuff in progress Pull requests that are not yet ready to be merged and issues that are being worked on. labels Jan 10, 2017
@ExE-Boss ExE-Boss linked a pull request Jan 10, 2017 that will close this issue
2 tasks
@ExE-Boss ExE-Boss added the task label Jan 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This is going to break stuff in progress Pull requests that are not yet ready to be merged and issues that are being worked on. plugin task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants