Skip to content

Add a message when MSBuild task or target skip due to condition evalu…#769

Merged
KirillOsenkov merged 3 commits intoKirillOsenkov:mainfrom
yuehuang010:dev/yuehuang/condition_compare
Apr 8, 2024
Merged

Add a message when MSBuild task or target skip due to condition evalu…#769
KirillOsenkov merged 3 commits intoKirillOsenkov:mainfrom
yuehuang010:dev/yuehuang/condition_compare

Conversation

@yuehuang010
Copy link
Copy Markdown
Contributor

Add a message explaining why the Condition evaluated to false. This parses evaluate the condition from the build message and prints which statement in the condition is false.

image

@KirillOsenkov
Copy link
Copy Markdown
Owner

I've been wanting to fix this in #389

@KirillOsenkov
Copy link
Copy Markdown
Owner

looks very cool!

@yuehuang010
Copy link
Copy Markdown
Contributor Author

yuehuang010 commented Apr 2, 2024 via email

Comment thread src/StructuredLogger/Strings/Strings.cs Outdated
public static Regex CreateRegex(string text, int replacePlaceholders = 0, RegexOptions options = RegexOptions.Compiled | RegexOptions.Singleline, bool capture = false)
{
text = Regex.Escape(text);
text = "^" + Regex.Escape(text) + "$";
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think we should check if these two symbols are already present. Also not sure if it's always safe to add these? I hope so?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added check. Though I think the Escape would invalidate it.

Comment thread src/StructuredLogger/Strings/Strings.cs Outdated
return;
}

Match matches;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Should this logic perhaps go into BuildControl.xaml.cs -> DisplayText() on line 2371?

Also maybe extract method so that this logic is a separate method.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

On the other hand perhaps keep it here, maybe in the future we can do smart highlighting to highlight substrings using the textEditor

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

still, I'd extract this logic to a separate method

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved to a separate function. I wanted to add Outlining to help visualize the position, but it might be too visually busy. Maybe in the future, it could have toggle button.

Comment thread src/StructuredLogger/Strings/Strings.cs Outdated
public static Regex CreateRegex(string text, int replacePlaceholders = 0, RegexOptions options = RegexOptions.Compiled | RegexOptions.Singleline, bool capture = false)
{
text = Regex.Escape(text);
if (text.StartsWith("^") || !text.EndsWith("$"))
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I don't quite understand this condition. Can you double-check?

I'd write this as:

text = Regex.Escape(text);
if (!text.StartsWith("^") && !text.EndsWith("$"))
{
    text = $"^{text}$";
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I haven't followed the conversation here, but I thought I'd point out that text.StartsWith("^") will always be false if text is an escaped regex.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am pretty sure that add ^$ is always safe, but I can't prove it, so I will just condition on the capture bool to limit the scope.

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