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

Modifications to test run parameter processing #1151

Merged
merged 5 commits into from
Mar 7, 2022
Merged

Conversation

CharliePoole
Copy link
Collaborator

Fixes #888

@jnm2 Since you worked on documenting the existing behavior I think you're the best one to review the changes I'm making.

}

[Test]
public void LeadingWhitespaceIsPreservedInParameterValue()
public void LeadingWhitespaceIsRemovedInParameterValue()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we want this behavior to change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seemed more like what one would expect. If you type X = 5, don't you expect X to be 5 rather than " 5" ?

Ideally I would have wanted your first example to work differently from the other two, but of course we get the same thing in all three cases after Windows command-line processing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not totally in love with this solution either. :-(

Copy link
Collaborator

@jnm2 jnm2 Feb 21, 2022

Choose a reason for hiding this comment

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

The problem is, what if someone types X=" 5" at the command line? When that gets turned into string[] using standard Windows argv rules, the string we get in the array is X= 5.

How then should people put leading or trailing spaces in test parameters if they want to?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(There's no way to distinguish whether "X= 5" or X=" 5" was typed by looking in string[] args.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, if someone typed X = 5 at the command line without quotes, string[] args would contain the three elements X, =, 5.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So... thinking about it, all of these same considerations apply in spades to the --where option, because there are even more possibilities as to how and where you use quotes on the command-line. Mostly, I've dealt with the problems people bring up by saying: "don't do that, do this." Can we do that here? The practical rules would be...

  1. Use =, : or space after --param
  2. Avoid spaces in the argument that follows, but
  3. If you must have spaces, put the whole following argument in quotes
  4. If the value part of the argument has spaces, use single or escaped double quotes around that part only.

So
--param X=5
--param="X = 5"
-p:"X='Something with spaces'" (OR ""Something with spaces""

Obviously, that would all have to be documented.

OTOH, we could go back to the existing behavior, which you documented in tests, but then we would be inconsistent with how --where is processed and up to now that has been OK.

Copy link
Collaborator

@jnm2 jnm2 Feb 22, 2022

Choose a reason for hiding this comment

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

The only downside to changing it is that wrappers of NUnit such as Cake and NUKE will not be able to share logic for escaping parameters between v3 and v4. If the difference is subtle, it might not get implemented properly. (For all I know, it might already not be.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the value part of the argument has spaces, use single or escaped double quotes around that part only.

Just as a reminder, with argv parsing, these three things all result in the string --param=X= 5 being in string[] args:

  • "--param=X= 5"
  • --param="X= 5"
  • --param=X=" 5"

We can't tell them apart. So we will either have to require single quotes, or never trim the value part. (Similar for the name part, but how likely is that to actually want whitespace around it?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's my point exactly. However, my conclusion from the same facts seems to be different. (No surprise)

If we could tell them apart, I'd want the third one to work differently from the first two. But we can't and I hope our users know that as well. (That last part is a bit shaky, since some of our users these days are not programmers.)

In any case, I'd like us to be consistent for all the options that take name/value pairs.

So which side are you coming down on here? Are you saying you're OK with trimming names but not values?

@CharliePoole
Copy link
Collaborator Author

@jnm2 OK... no more updates till you finish!!!

@CharliePoole
Copy link
Collaborator Author

Rebased. @jnm2 Where are we on this... i.e. your thoughts on trimming names and/or values?

@CharliePoole
Copy link
Collaborator Author

I'd like to keep up the momentum so I'll merge this now but I'm open to further changes when we have time to discuss them.

@CharliePoole CharliePoole merged commit d54f85c into main Mar 7, 2022
@CharliePoole CharliePoole deleted the issue-888 branch March 7, 2022 20:34
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.

v4: Remove deprecated --params option
2 participants