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

feat: add input_hidden function #492

Merged
merged 6 commits into from
Oct 1, 2024

Conversation

karpfediem
Copy link
Contributor

Adds input_hidden function to std/env

Prompts for input, but hides the user input (useful for password prompts)

Copy link
Member

@Mte90 Mte90 left a comment

Choose a reason for hiding this comment

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

We need a test :-)

@karpfediem
Copy link
Contributor Author

I copied the existing test of the input function.

I found a weird issue when running the test. It prints the following messages into stderr when calling stty


[carp@aquarium:~/code/clones/amber/src/tests/stdlib]$ ./input_hidden.sh 
stty: 'standard input': Inappropriate ioctl for device
Please enter your name:stty: 'standard input': Inappropriate ioctl for device

Hello, Amber

[carp@aquarium:~/code/clones/amber/src/tests/stdlib]$ ./input_hidden.sh 2>err.txt
Please enter your name:
Hello, Amber

[carp@aquarium:~/code/clones/amber/src/tests/stdlib]$ cat err.txt 
stty: 'standard input': Inappropriate ioctl for device
stty: 'standard input': Inappropriate ioctl for device

However it works fine even in that case.

I need some help to figure out if thats okay, or should be surpressed somehow or anything.

@Mte90
Copy link
Member

Mte90 commented Sep 26, 2024

Let's see what happens in the CI

@Ph0enixKM Ph0enixKM self-requested a review September 27, 2024 16:47
Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

stty: 'standard input': Inappropriate ioctl for device
Please enter your name:stty: 'standard input': Inappropriate ioctl for device

Have you tried read -s -p "The prompt"?

I've tested this and it works perfectly. But since input uses read command I think it would be nice if we used read command for input_hidden too.

src/std/env.ab Show resolved Hide resolved
src/tests/stdlib/input_hidden.ab Outdated Show resolved Hide resolved
@karpfediem
Copy link
Contributor Author

If i run the test now

main {
    unsafe $echo "Amber" >> /tmp/test_input$
    unsafe $exec 0< /tmp/test_input$
    let name = input_hidden("Please enter your name:")
    echo "Hello, " + name
    unsafe $rm /tmp/test_input$
}

outputs just Hello, Amber

Running this

 main {
     let name = input_hidden("Please enter your name:")
     echo "Hello, " + name
 }

turns it interactive again and outputs Please enter your name:Hello, Amber

Which is missing the linebreak. I'll look into this again in a bit.

@karpfediem
Copy link
Contributor Author

karpfediem commented Sep 27, 2024

So the issue with read -p is that the prompt gets redirected to fd 2 and that it omits the newline.

I'm not sure if

  • the prompt itself should be in stdout (currently for input it is, because it doesn't use read -p)

I would like to

  • avoid the newline before the prompt is actually confirmed by the user
  • still send a newline after the prompt was written
  • send the newline to the same output as the prompt in any case

EDIT: maybe i've just got the wrong idea and we don't want to send a newline at all, just leave it up for the end user...

@hdwalters
Copy link
Contributor

So the issue with read -p is that the prompt gets redirected to fd 2 and that it omits the newline...

Assuming you're planning to modify input as well, I copied the functions to this test script:

$ cat prompt.ab
#!/usr/bin/env amber

pub fun input(prompt: Text): Text {
    unsafe $read -p "\${nameof prompt}"$
    return "\$REPLY"
}

pub fun input_hidden(prompt: Text): Text {
    unsafe $read -s -p "\${nameof prompt}"$
    return "\$REPLY"
}

echo "[before input]"
let password = input("Password: ")
echo "[after input]"
echo password

echo "[before input_hidden]"
let password = input_hidden("Password: ")
echo "[after input_hidden]"
echo password

The input function seems to work fine, but input_hidden does not:

$ ./prompt.ab
[before input]
Password: SHOWN
[after input]
SHOWN
[before input_hidden]
Password: [after input_hidden]
HIDDEN

Why can't you just add an artificial newline?

pub fun input_hidden(prompt: Text): Text {
    unsafe $read -s -p "\${nameof prompt}"$
    echo ""
    return "\$REPLY"
}

Seems to fix the problem:

$ ./prompt.ab
[before input]
Password: SHOWN
[after input]
SHOWN
[before input_hidden]
Password: 
[after input_hidden]
HIDDEN

@karpfediem
Copy link
Contributor Author

Thanks :)

I'm fine with that solution, as long as both input functions are handled equally.
I wouldn't want to decide on my own if we should put a forced newline or not though.

So waiting for other opinions on that

Copy link
Contributor

@hdwalters hdwalters left a comment

Choose a reason for hiding this comment

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

Looks good.

@b1ek
Copy link
Member

b1ek commented Sep 30, 2024

@Ph0enixKM can we merge this?

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

Tested and works great

@Ph0enixKM Ph0enixKM merged commit 1ab2bd9 into amber-lang:master Oct 1, 2024
1 check passed
@karpfediem karpfediem deleted the feature/input_hidden branch October 1, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants