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

Escape the build arguments containing backslash #1593

Closed
wants to merge 2 commits into from
Closed

Escape the build arguments containing backslash #1593

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 11, 2020

Fixes the following issue with passing build arguments
System.FormatException in AfterAssemblyLoadingAttached

@dnfadmin
Copy link

dnfadmin commented Nov 11, 2020

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ ghost sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

@ghost ghost closed this Nov 11, 2020
@ghost ghost deleted the dotnet/BenchmarkDotNet/issues/1536 branch November 11, 2020 22:06
@ghost ghost restored the dotnet/BenchmarkDotNet/issues/1536 branch November 11, 2020 22:28
@ghost ghost reopened this Nov 11, 2020
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

Hi @VijayakumarNatarajan

Big thanks for your PR!

Overall it looks good, but please address the comments that I've added before I merge the PR.

Thanks,
Adam

Comment on lines 43 to 44
/// <param name="args"></param>
/// <returns></returns>
Copy link
Member

Choose a reason for hiding this comment

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

nit: we try to avoid empty descriptions, so this can be just removed

Suggested change
/// <param name="args"></param>
/// <returns></returns>

Copy link
Author

Choose a reason for hiding this comment

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

Removed the description from the documentation section

/// </summary>
/// <param name="args"></param>
/// <returns></returns>
public string EscapeBuildArguments(string args)
Copy link
Member

Choose a reason for hiding this comment

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

could you please make this method internal and add some simple unit test for it?

Copy link
Author

Choose a reason for hiding this comment

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

Marked the method as internal instead of public

/// <returns></returns>
public string EscapeBuildArguments(string args)
{
if (args.Contains("\\"))
Copy link
Member

Choose a reason for hiding this comment

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

we should also consider the case where the user provides an already escaped characters like new MsBuildArgument("/p:a=b\\\\") and to be 100% complete a version when there is a mix like ("/p:\a=b\\\\")`

Copy link
Author

Choose a reason for hiding this comment

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

Added unit test cases to cover the scenarios as suggested

Added unit test cases for escaping MSBuildArguments
@ghost
Copy link
Author

ghost commented Nov 13, 2020

@adamsitnik
Resolved the review comments.
Could you please review them?

@ghost ghost requested a review from adamsitnik November 15, 2020 16:22
@ghost
Copy link
Author

ghost commented Nov 19, 2020

@adamsitnik could you please review the PR?

@ghost ghost closed this Nov 27, 2020
This pull request was closed.
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.

3 participants