Added the ability to use \n and/or \r in HelpText

[rupe120]

It would be helpful to be able to create more than one paragraph in the HelpText, for the instances where there is supplemental / conditional information to add.

I have a scenario where depending on the list of options supplied, a given option may have slightly different behavior and/or be required or not.

It tends to be helpful if the user can see clear separation between the default functionality of an option, and additional scenarios.

[gsscoder]

I've added comments to the PR #168 that you closed... This is the new version of the PR?
If yes, can I ask you to add a test relative to changes?

[rupe120]

Yes this is a new version, I apologize for the confusion and I will add a test

[gsscoder]

@rupe120, no problem... 👍 If you need to ask something, don't hesitate.

[rupe120]

Thanks! Will do

On Tue, Jun 23, 2015 at 9:16 AM, Giacomo Stelluti Scala <
[email protected]> wrote:

@rupe120 https://github.com/rupe120, no problem... [image: 👍] If you
need to ask something, don't hesitate.

—
Reply to this email directly or view it on GitHub
#169 (comment).

[gsscoder]

So it's OK, I wait you test addition and if possible (and you've a time) a sample (side by side) of current help output and what you want to get.

Have a nice day!

[rupe120]

Np, the sample should be fairly easy

On Tue, Jun 23, 2015 at 2:15 PM, Giacomo Stelluti Scala <
[email protected]> wrote:

So it's OK, I wait you test addition and if possible (and you've a time) a
sample (side by side) of current help output and what you want to get.

Have a nice day!

—
Reply to this email directly or view it on GitHub
#169 (comment).

[rupe120]

Hey guys, heads up on xunit. They've changed their runner and switched from an extension to a NuGet package http://xunit.github.io/docs/running-tests-in-vs.html

[rupe120]

The before and after

[rupe120]

btw, I actually removed \r from the processing because I thought it would be confusing to see \r\n and actually receive 2 lines. While I did want \n\n to give you 2 lines, the distinction would be more logic then I felt necessary. So I just eliminated the \r

[indigotock]

Would using Environment.NewLine not be better?

[gsscoder]

@indigotock, yes, is also consistent with the rest of HelpText class. @rupe120, can you do this last change to your PR, please?

(If you've done in it, please forgive me, have you verified if after the change all tests are green?)

Ping me when you're done, I'll fetch the PR locally to proceed.

[rupe120]

Ok so, I agree that we need to keep everything consistent. On the other hand, I'm seeing this context as being different from the other places where Environment.NewLine is used. For all other places in the code the Environment.NewLine reference is used in the background to create the string that will be fed to the console. In this instance our interface is between the developer in what they have typed for help text and the help text processing, instead of the help text processing and the console.

If I use the Environment.NewLine instead of a raw \n then the users will need to use Environment.NewLine in their help text strings as well. It seems to me that using \n will be an easier interface for the developer to use.

Thoughts?

[rupe120]

With that said, I agree that there should be a comment next to the .split() call that uses \n explaining why the break in pattern because anyone else looking at the code will have the same question.

[indigotock]

I think that users implementing new lines "incorrectly" is something which shouldn't be counted on by the library? Quite interested to hear other peoples' opinions on this. Perhaps there could be an enum flag which allows the author to specify the new line sequence? Something like

enum NewLineStyle{
    LineFeed,
    CarriageReturnLineFeed,
    EnvironmentNewLine,
    Any
}

Although this feel a little bit over-engineered

[rupe120]

Though to do this "correctly" would make for more awkward HelpText creation, either using String.Format() or concatenation

[rupe120]

I think it's more developer friendly to view attribute property text as a version of formatted text, to minimize the logic they need to surround the string with

[rupe120]

But that's just me :o)

[alexanderfast]

Usually when I tackle the issue of platform dependent new line characters I choose one to use internally, usually \n, then everywhere input is read and Environment.NewLine is not equal to \n I coerce. And in the same way everywhere output is written to Console I also coerce.

Here we have a bit of a corner case because a user of the library would supply text with newlines that shouldn't (imo) be coerced and has to know to stick to \n or whatever internal representation we decide upon.

@rupe120 Please edit your existing post if you feel you need to amend something, you created 3 posts in 6 minutes.

[indigotock]

It's just occurred to me that C# can split strings with arrays of strings, not just chars (not sure on the performance of this):

Split(new string[] { "\r\n", "\n", Environment.NewLine }, StringSplitOptions.Whatever? )

Failing that, one could replace all occurances of Environment.NewLine with \n before processing, and keep splitting on \n

[rupe120]

@mizipzor My apologies for the comment spam. Will do.

@indigotock I can do that, and I think that might actually lead to more expected and intuitive behavior. In addition, I also believe the array argument is the default operation of split so it should be just as performant, that and we aren't really dealing with massive amounts of text here.

I vote for @indigotock's string array suggestion

[indigotock]

An issue with that is I'm now sure how it will handle splitting on \n when \r\n is in the string, I'm assuming it iterates over the array in order but you'd have to test it

[alexanderfast]

I ran across this problem as well needing to add the xunit.runner dependency. Can we get this as a separate pull request (I can do it)? I would like this merged right away, separate from this discussion.

[rupe120]

Ya, that was part of my comments above. I thought you might want to do that.

@mizipzor Should I negate that from my branch to remove it from this pull request when my tests are complete?

[alexanderfast]

I just pushed this in 4a155be, so just rebase this branch on top of master and that line should go away (since it is already added). I think we prefer rebases over merges to keep a linear history but I'm not sure so we might want a comment from @gsscoder here.

[rupe120]

That's works for me. That just means I don't need to do anything :o)

[rupe120]

I'll test it, but I believe if you have \r\n before \n it will work as expected

[alexanderfast]

Spotted another issue, assuming we are trying to get this merged, the file CommandLine.sln.DotSettings.user shouldn't be commited. That is ReSharper settings specific for you and you alone. The corresponding file for shared settings is called CommandLine.sln.DotSettings and that can be (and should be) commited. It doesn't exist in this repository right now, I added it as part of my cleanup branch.

[rupe120]

I'm not sure I grock exactly what's going on here but the end result is, the order doesn't matter for the different new line styles in the array argument for the split method

[rupe120]

Let me know how that works.

Also note that I added a .gitignore line to exclude *.DotSettings.user

[alexanderfast]

While you added an ignore for the file the file itself is still commited. :)

[rupe120]

Ya, it was to metaphorically stop the bleeding. How do you remove a file from a repo?

Is the only way to:

• undo my .gitignore update
• remove the file from the project scope
• commit the fact that it's no longer there
• re-do the .gitignore setting

[alexanderfast]

Yes, that is the simple and obvious way to do it. Though this is a good opportunity to learn some advanced Github. If you want, here's what I would do. Note that all this happens locally, everything currently online serves as a backup, so go nuts.

Edit: It's important to have the branch you want to rebase checked out:
git checkout master

git rebase -i 90291d4^ or whatever the hash of the commit we want to change

You now get a document in your default text editor. Here's how my look (I fetched your branch):

pick 90291d4 added test with line feeds
pick 68252d1 added the ability to use either Unix or Windows new lines in help text
pick a7005ad removed DotSettings.user files
pick 45dec68 re-added the gitignore for DotSettings.user

We want to change this into the following:

edit 90291d4 added test with line feeds
pick 68252d1 added the ability to use either Unix or Windows new lines in help text
pick 45dec68 re-added the gitignore for DotSettings.user

Note the word edit, we are going to change that commit. And one commit has been removed entirely. Save and close the document.

In the console now you should see that Git is waiting for you to edit the relevant commit. Open the folder and delete the .user file(s). Then:

git commit --all --amend --no-edit
git rebase --continue

Git will now complain about a commit being empty since we've changed stuff around. Do:

git reset
get rebase --continue

That should be it. 😃 At this point look over your commits. Up until now all has been locally, until you push this everything online serves as a backup. To force push this replacing the current commits in the branch:

git push --force

Here's some extra documentation if you like reading. As an example and making sure it works I did this locally and pushed as a branch on my fork. An interesting tidbit here that I like is that we are both listed on the commit, you're the author and I'm commiter, since I've changed it. Even if I would do cleanup, you still get credit.

You now know more Git. Gratz.

[gsscoder]

@mizipzor, this is useful to me too! 👍

[rupe120]

@mizipzor These are some awesome Git ninja moves! I think I'm going to need to read through a couple times to really get my head around it. Thank you so much!

[alexanderfast]

Updated the comment with a step (the checkout) that I didn't include initially. Another reminder also that if you screw up something while doing this the branch is still online so can just nuke your local clone and re-download your fork. And @gsscoder, if you want to try this you don't have to wait for a an opportunity, you can do what I did which is do pull this branch specifically and do the rebase cleanup.

[gsscoder]

@mizipzor, just one thing... I was unable to understand the word nuke when you say:

...so can just nuke your local clone...

Can you give me a synonymous? Thanks.

[alexanderfast]

Sure. Nuke as in delete the repository folder from your hard drive and start over, like it never was there. Doing the whole git clone thing again. I sometimes do this when trying out destructive operations. I push my branch to Github to serve as a backup and if a screw up it's usually faster/easier to just re-clone than trying to undo whatever I did.

[gsscoder]

@mizipzor, OK, now it's clear.