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

Read Prompt Support #722

Merged
merged 4 commits into from
Aug 26, 2021
Merged

Conversation

parkerduckworth
Copy link
Contributor

@parkerduckworth parkerduckworth commented Aug 17, 2021

Hey @mvdan 👋

I have implemented the read -p flag, as requested in issue #551. Included are new test cases to cover the added feature, as well as the integration of -p with the existing code that handles read. All test cases old and new are passing.

Let me know if there are any changes still needed, and thanks for your hard work maintaining this library!

Copy link
Owner

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

Appreciated! Just one comment to simplify the flag parsing logic.

@@ -538,6 +544,14 @@ func (r *Runner) builtinCode(ctx context.Context, pos syntax.Pos, name string, a
}
}

if prompt.provided {
if prompt.message == "" {
Copy link
Owner

Choose a reason for hiding this comment

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

you could make this check inside case "-p", then you don't need provided at all - you'd just do message != "".

and you could also just do message := ""; I don't think the struct is necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok gotcha, making the change 👍

@parkerduckworth
Copy link
Contributor Author

@mvdan I left the if statement checking to see if prompt was provided before writing to stdout. Although this check is not strictly necessary, it seems worth it to prevent the unnecessary IO, but can remove it if you prefer.

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.

2 participants