comments (?) behaviour about readline HOT 14 CLOSED

Hello @lucapette , glad to jear you enjoy this code.

It's a bug (logic is wrong in my code), will fix it soon. If you can check it before and/or suggest a fix I would appreciate this greatly !

Will get in touch very soon, I'm a little busy at the moment.

from readline.

If this code is realistic example of how the C lib handles input (regarding comments of course):

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <readline/readline.h>

int main() {
    char *input;

    while (1) {
        input = readline("> ");

        if (input == NULL || strcmp(input, "exit") == 0) {
            free(input);
            break;
        }

        printf("%s\n", input);

        free(input);
    }

    return 0;
}

Then we'd have to conclude, the C lib doesn't handle comments for the user. Test run:

> hello
hello
> "#hello"
"#hello"
> #hello
#hello
> " #{hello}"
" #{hello}"
> 

Obviously this was a super quick (and superficial) test. Not sure it's enough to take a decision on how to move forward (probably not :D)

from readline.

Thank you for these pointers! I'll see what I can do tomorrow 💪

from readline.

Hello @lucapette, I will deal with this in the current week.

from readline.

Fixed the problem in #58 , see my last comment in #55.

from readline.

oh thank you very much for taking care of this!

In the meanwhile, I think I found some other behaviour I can't tell if it's a bug or a feature. I did not have the time to open an issue because this one seems harder to reproduce.

I'll get back to it early next week and should be able to share more then. I'll try to get more familiar with the code base as well as I'd love to help you a little more!

from readline.

OK, I'll see if I can figure this out myself tomorrow morning (I'm using readline for the official CLI of typestream.io and the bug prevents me to share some demos so I've got some motivation :))

from readline.

my plans for the evening changed and I had the chance to have a look now. I'm not 100% sure (as there's obviously a lot going on inside a rl.Readline() call) but I think the problem may be the regex used inside the "LineAccepted()" method to trim out comments.

I wrote a tiny program to reproduce that code:

package main

import (
	"fmt"
	"regexp"
	"strings"
)

func main() {
	lines := []string{
		"hello",
		"\" #hello\"",
		"\"#{hello}\"",
		"\" #{hello}\"",
		"\"#{hello} \"",
	}

	for _, line := range lines {
		newLine := line
		comment := strings.Trim("#", "\"")
		commentPattern := fmt.Sprintf(`%s.*$`, comment)

		if commentsMatch, err := regexp.Compile(commentPattern); err == nil {
			newLine = commentsMatch.ReplaceAllString(newLine, "")
		}

		fmt.Printf("%s --> %s\n", line, newLine)
	}
}

this prints:

hello --> hello
" #hello" --> "
"#{hello}" --> "
" #{hello}" --> "
"#{hello} " --> "

As for a fix, I'm not sure it's possible to fix this with a different regex. I believe you'd need to tokenize the input to make sense of it. That seems a tad overkill "just" to clean up the line from comments though :/

from readline.

Okay so indeed I'm using a regular expression for this, as like you mentioned, tokenizing the line is out of the library scope.

Now obviously there is something wrong here, which is that either readline is taking care of something (trimming comments) that it shouldn't, or it should do it in a more reliable way.

Either way I'm open to thoughts on this. My first two cents:

• regular expressions seem to be the simplest way to deal with such things, however and as you saw, quoting is an issue. Maybe that makes a regex a no-go for this.

• I would look at how the original GNU C readline code handles that, maybe with some luck that logic is contained within a single C code file.

from readline.

Now obviously there is something wrong here, which is that either readline is taking care of something (trimming comments) that it shouldn't, or it should do it in a more reliable way.

at first I found it counterintuitive that readline handles comments but to be fair I have no previous experience with this (first time I'm building something "shell like") and can't tell if handling comments should be readline responsibility.

As you say, best is to look at the C "reference implementation". I haven't done any C in a decade or so, I'll see what I can do :)

from readline.

Thanks a lot for this quick check, which was definitely smarter than looking at the C codebase itself.

So the conclusion is easy here, as you noted the readline returns the entire input buffer including comments.

Another set of thoughts/alternatives on this:

1. letting the user choose as an .inputrc option whether or not to return the comments is a no-go, as it's never the end-user who decides what to do with the return buffer (and how to do it).
2. This could perhaps be let configurable through the API, so the library users are free to choose either.
3. This library could simply follow the reference implementation and return the comments as well, in which case some documentation on how to handle them after return would be welcome by most.

So technically either 2)or 3) list items above are possible. I would personally lean for 2), and add the documentation from 3).

from readline.

if I had to choose, I'd also choose 2 I think. You want me to take care of it? I can do if you tell me a bit more about how you imagine the api to look like (so we shorten the feedback loop)

from readline.

I'm unable to code at the moment, so if you can take a shot at it I would appreciate that (even if you don't go all the way).

So my approach would be:

1. Add an option in this file: https://github.com/reeflective/readline/blob/master/inputrc/parse.go
   Similarly to WithApp, WithTerm etc, except that this won't be related to parsing the configuration .inputrc file.
   I would probably name it WithTrimmedBufferComments(), because a shorter WithTrimmedComments will be ambiguous with regards to the configuration comments.
   You will probably need to add an internal struct field to the parser to store this boolean.

2. Then, in https://github.com/reeflective/readline/blob/master/shell.go, and https://github.com/reeflective/readline/blob/master/readline.go, you'll find where this option can be stored (probably in the shell.Histories struct component, in which you could add this same boolean and act upon it when accepting the readline buffer.

You will have understood it: the point is to let the user specify this behavior only when instantiating a new shell, through the readline.NewShell(...inputrc.Option) variadic parameter. This way the API remains compatible.
Then the goal is to pass this option/boolean down to the component that needs it (most probably the Histories one (see the internal/history package).

Ask anything if you need further help.

from readline.

so personal commitments got in the way of this and could have a look only this morning.

I added the option you suggested (and fixed little typos while reading).

Now I'm using h.config.Get('trim-comments') in the LineAccepted method but the work isn't done because it's not clear to me how to convert the Option into a Config (tried to look for similar approaches but got lost).

Questions:

• is this h.config.Get the right way to proceed?
• I can add, as you suggested, the same bool to the Sources struct (which you're calling Histories in your comment... either I can't find the right one or that's a typo?) but it feels wrong to change NewSources api for this bool (which is why I'm looking to convert the option into a config and don't know how to do it).

I updated the WIP open pr with the little code I wrote so that it's easier to understand what I just wrote. Would love to finish this myself with more pointers :)

from readline.