Design documentation.

[Liam Coughlin]

Is there like - some design documentation somewhere?

I'm having difficulty understanding some of the structures.

Like org.jline.builtins.Widgets.

This class seems to be pulling double duty as a name space AND a base class a base class and it's constantly confusing me.

I feel like there should be a package named "org.jline.widgets" (why is this in builtins?) that contains the Widget class, and then all of it's Children.

And my confusion just gets worse when i get to ConsoleEngine and what not.  I feel like maybe there's something that I'm missing about how these structures are built up maybe?

FYI - I love the project, and it's super impressive - i'm just trying to figure it out is all.

Cheers

-L

[Matti Rinta-Nikkola]

Thanks for the feedback Liam!

I do not think JLine has design documentions...

My implementations on JLine have started as small experimentations without me having an idea how they would grow in complexity. For example Widgets implementation could have ended a few lines of code that would have no problem to be part of builtins package. I did not want to create a new package for it when I started to implement it. I have understood your point and actually I have even noticed the problem myself :) but I have postponed to resolve it.

As JLine project is composed by small modules I think we could create a new module. We could start to resolve the problem by splitting builtins in two modules: the actual builtins and a new console module. I think that the console module could contain the implementations of Widgets, ConsoleEngine and SystemRegistry.

Matti

[Liam Coughlin]

Fair enough.

Is there some reason why CommandMethods has both a consumer and a function.

What is the difference between SystemRegistryImpl.invoke and SystemRegistryImpl..execute?

Shouldn't execute return the result of the command it executed?

Is this a rational patch?
https://github.com/lscoughlin/jline3/commit/cdfbc18f46a36c7383f08c87459ff78f6388c8a7

--
You received this message because you are subscribed to the Google Groups "jline-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jline-users...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jline-users/8c2bb2fa-543a-4f25-8f96-b43e3fa5c77e%40googlegroups.com.

[Matti Rinta-Nikkola]

Your patch look good to me!

Some observations and explanations:

1) I would not like the change the name of the CommandMethods because it will have two methods like before one for command execution and an other to compile command tab completers. Change would require also unnecessary modification for library users.

2) All the JLine builtin commands are void methods with string arguments and that's why there is execute() method with a consumer. When you use for example Groovy repl you can create commands that can return objects and have object arguments that's why there is invoke() method with a function. That's more or less why in CommandMethods class has ended to have possibility to host both consumers and functions. That duplication can be avoided by applying your patch.

3) You should remove execute() method from CommandRegistry interface and from its implementations. Of course also CommandRegistry interface default implementation of invoke() method should be removed.

4) You should remove isConsumer() method from CommandMethods class. All what we have are functions and they are treated in the same way.

5) Would be great if squash your commits in single one in your pull request.

Thanks,

Matti

Il giorno sabato 30 maggio 2020 18:18:11 UTC+2, Liam Coughlin ha scritto:

Fair enough.

Is there some reason why CommandMethods has both a consumer and a function.

What is the difference between SystemRegistryImpl.invoke and SystemRegistryImpl..execute?

Shouldn't execute return the result of the command it executed?

Is this a rational patch?
https://github.com/lscoughlin/jline3/commit/cdfbc18f46a36c7383f08c87459ff78f6388c8a7

On Sat, May 30, 2020 at 11:17 AM Matti Rinta-Nikkola <matti.ri...@gmail.com> wrote:

Thanks for the feedback Liam!

I do not think JLine has design documentions...

My implementations on JLine have started as small experimentations without me having an idea how they would grow in complexity. For example Widgets implementation could have ended a few lines of code that would have no problem to be part of builtins package. I did not want to create a new package for it when I started to implement it. I have understood your point and actually I have even noticed the problem myself :) but I have postponed to resolve it.

As JLine project is composed by small modules I think we could create a new module. We could start to resolve the problem by splitting builtins in two modules: the actual builtins and a new console module. I think that the console module could contain the implementations of Widgets, ConsoleEngine and SystemRegistry.

Matti

Il giorno mercoledì 27 maggio 2020 16:08:11 UTC+2, Liam Coughlin ha scritto:

Is there like - some design documentation somewhere?

I'm having difficulty understanding some of the structures.

Like org.jline.builtins.Widgets.

This class seems to be pulling double duty as a name space AND a base class a base class and it's constantly confusing me.

I feel like there should be a package named "org.jline.widgets" (why is this in builtins?) that contains the Widget class, and then all of it's Children.

And my confusion just gets worse when i get to ConsoleEngine and what not.  I feel like maybe there's something that I'm missing about how these structures are built up maybe?

FYI - I love the project, and it's super impressive - i'm just trying to figure it out is all.

Cheers

-L

--
You received this message because you are subscribed to the Google Groups "jline-users" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jline...@googlegroups.com.

[Liam Coughlin]

Thanks - I'll do those things :)

I do have one comment though because i'm not sure what the impact is.  There is this block of code in AbstractCommandRegistry:

if (methods.isConsumer()) {
    String[] _args = new String[args.length];
    for (int i = 0; i < args.length; i++) {
        if (!(args[i] instanceof String)) {
            throw new IllegalArgumentException();
        }
        _args[i] = args[i].toString();
    }
    methods.execute().accept(new CommandInput(command, _args, session));
} else {

Where in it converts the args to a strings or throws an illegal argument exception.  While it looks unnecessary to me, it also looks like it was done for a reason, so I thought I would ask.

Cheers

-L

To unsubscribe from this group and stop receiving emails from it, send an email to jline-users...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jline-users/f5a15562-dbc5-4a77-a7f4-c9b0bc1ee84a%40googlegroups.com.

[Liam Coughlin]

Are there formatting standards?

There seem to be some monitor differences here and there when I format something, particularly around imports that seems to make things a bit messy in diffs.

On Mon, Jun 1, 2020 at 3:33 PM Matti Rinta-Nikkola <matti.rin...@gmail.com> wrote:

Code block below is not needed we have only Functions....  

Il giorno lunedì 1 giugno 2020 13:32:26 UTC+2, Liam Coughlin ha scritto:

Thanks - I'll do those things :)

I do have one comment though because i'm not sure what the impact is.  There is this block of code in AbstractCommandRegistry:

if (methods.isConsumer()) {
    String[] _args = new String[args.length];
    for (int i = 0; i < args.length; i++) {
        if (!(args[i] instanceof String)) {
            throw new IllegalArgumentException();
        }
        _args[i] = args[i].toString();
    }
    methods.execute().accept(new CommandInput(command, _args, session));
} else {

Where in it converts the args to a strings or throws an illegal argument exception.  While it looks unnecessary to me, it also looks like it was done for a reason, so I thought I would ask.

Cheers

-L

To view this discussion on the web visit https://groups.google.com/d/msgid/jline-users/f5a15562-dbc5-4a77-a7f4-c9b0bc1ee84a%40googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "jline-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jline-users...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jline-users/caa5844e-6059-4475-9787-dd07f6b5eef6%40googlegroups.com.