Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Added ability to provide a default value. #8

Merged
merged 6 commits into from
Feb 21, 2017

Conversation

DewJunkie
Copy link
Contributor

Possibly this is out of scope, but I started using default values recently in my console programs, I don't know if this falls into the works like GNU readline, because I haven't used it as a developer, but I have noticed this used in other programs that I have used.
I will probably add as a static property a prompt suffix. ex ":", ">" or whatever suits the app.

This is my first pull request, so please point out anything wrong I did in the process if you have the time.

Thanks

@tonerdo
Copy link
Owner

tonerdo commented Feb 16, 2017

@DewJunkie thanks for this PR and I can understand what you're trying to do and how it will add value. However, how does this feature compare to a user just doing this:

string input = ReadLine.Read("Would you like to continue? [Y]:");
if (string.IsNullOrWhitespace(input))
    ....
else
    ....

@DewJunkie
Copy link
Contributor Author

DewJunkie commented Feb 16, 2017

It is exactly the same as what you mentioned, so it doesn't add anything not possible, just cleans it up. I use a bit of console apps for testing/validation. I have some users who just want to double click anything I send them, so my typical pattern is.

app.exe --option1 a --option2 b --option3 c settings.json

// Loop
{
  // Show app menu
  app.option1 = ReadLine.Read("Prompt", app.option1);
  // etc...
}

So if the goal is be like GNU readline, I could see it being out of scope. My goal was, make my life easier with minimal change upstream. It's totally understandable if they don't align.

Copy link
Owner

@tonerdo tonerdo left a comment

Choose a reason for hiding this comment

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

After giving it a bit of thought I figured that having a defaultInput will be a nice feature to have. However, let's not tie the developer in with a specific prompt closer or defaultInput print out format

/// ^
/// </summary>
public static string PromptCloser = ":";

Copy link
Owner

Choose a reason for hiding this comment

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

Let's remove the PromptCloser

{
Console.Write($"[{defaultInput}]");
}
Console.Write(PromptCloser);
Copy link
Owner

Choose a reason for hiding this comment

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

No need to print out the default input or prompt closer let the developer specify the output string format using the prompt parameter

else
{
_history.Add(text);
}
Copy link
Owner

Choose a reason for hiding this comment

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

The defaultInput isn't being added to the history. Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it is written "text" would be an empty string, I felt like adding the default to the history didn't make sense, and adding an empty value also would not make sense, and that is why I omit adding anything to history if the default value is used. If we were going to go with 1 of the 2 options, adding the default value would be the better choice, but then hitting (up) would have something that you never typed in.

@tonerdo
Copy link
Owner

tonerdo commented Feb 16, 2017

@DewJunkie This feature is super welcome I just have a couple of comments. I suggest you resolve merge conflicts first so that your changes are made on updated code and the CI tests can validate this PR

@tonerdo
Copy link
Owner

tonerdo commented Feb 19, 2017

Are you still looking into this? /cc @DewJunkie

@DewJunkie
Copy link
Contributor Author

I was/am travelling for work, I am going to try and have the suggestions you made completed/checked in either later tonight, but possibly it will be the end of the week.

@DewJunkie
Copy link
Contributor Author

Ok I merged in the latest, and made the changes as discussed. I don't know if the pull request is tied to a particular commit, or if it is from my master branch. If I need to submit a new pull request and cancel this one, let me know.

@@ -0,0 +1,19 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Owner

Choose a reason for hiding this comment

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

Let's just use project.json for now. This project will migrate to csproj when the dotnet cli comes out of preview

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed as suggested.

.gitignore Outdated
project.lock.json
/src/ReadLine/*.user
Copy link
Owner

Choose a reason for hiding this comment

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

Just *.user should be sufficient and would cover all places it could occur in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modified as suggested

@tonerdo tonerdo merged commit 5a174be into tonerdo:master Feb 21, 2017
@tonerdo
Copy link
Owner

tonerdo commented Feb 21, 2017

Thanks @DewJunkie 👍

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants