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

[FancyLogger] Show link to project outputs #8324

Merged
merged 23 commits into from
Feb 7, 2023

Conversation

edvilme
Copy link
Member

@edvilme edvilme commented Jan 19, 2023

Fixes #

Context

One of the key features of the old console logger is showing the path to the output file for each project using the format
Project -> path/to/output.
To save space, FancyLogger only shows the filename of the output in a clickable link that redirects to the output file following the same format.
image

Changes Made

  • Updated ANSIBuilder.Formatting.Hyperlink to support clickable hyperlinks
  • Updated Regex for matching ANSI codes to also match hyperlinks
  • TFMs now show in a white background

Testing

Notes

@edvilme edvilme requested a review from Forgind January 20, 2023 02:35
@rainersigwald
Copy link
Member

This is really slick, but I think it hurts a critical use case: getting a path to an output so I can run it.

I often do

  1. dotnet build
  2. double-click output path message to get full path to a specific output
  3. right click (copy)
  4. right click (paste)
  5. enter (run that thing)

I find this easier than dotnet run because you can build a whole solution and because you can iterate on adding args to the thing more quickly and easily.

Fortunately I think there's an easy fix: don't truncate the path, but still hyperlink it. Then I can copy/paste but also see it in Explorer.

@edvilme
Copy link
Member Author

edvilme commented Jan 20, 2023

This is really slick, but I think it hurts a critical use case: getting a path to an output so I can run it.

I often do

  1. dotnet build

  2. double-click output path message to get full path to a specific output

  3. right click (copy)

  4. right click (paste)

  5. enter (run that thing)

I find this easier than dotnet run because you can build a whole solution and because you can iterate on adding args to the thing more quickly and easily.

Fortunately I think there's an easy fix: don't truncate the path, but still hyperlink it. Then I can copy/paste but also see it in Explorer.

Alright. Do you think we should clip them if they're too long?

@rainersigwald
Copy link
Member

Do you think we should clip them if they're too long?

I'm torn on this but lean toward "no, output the whole path". Would happily receive feedback!

@@ -10,10 +10,45 @@ namespace Microsoft.Build.Logging.FancyLogger
{
internal static class ANSIBuilder
{
public static string ANSIRegex = @"\x1b(?:[@-Z\-_]|\[[0-?]*[ -\/]*[@-~])";
public static string ANSIRegex = @"\x1b(?:[@-Z\-_]|\[[0-?]*[ -\/]*[@-~]|(?:\]8;;.*?\x1b\\))";
Copy link
Member

Choose a reason for hiding this comment

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

For follow-up PR: this should be a static Regex constructed with RegexOptions.Compiled, which should dramatically reduce overhead of scanning using it. On .NET 7, we should use a [GeneratedRegex].

src/Build/Logging/FancyLogger/ANSIBuilder.cs Outdated Show resolved Hide resolved
src/Build/Logging/FancyLogger/ANSIBuilder.cs Outdated Show resolved Hide resolved
src/Build/Logging/FancyLogger/ANSIBuilder.cs Outdated Show resolved Hide resolved
src/Build/Logging/FancyLogger/ANSIBuilder.cs Outdated Show resolved Hide resolved
src/Build/Logging/FancyLogger/FancyLogger.cs Outdated Show resolved Hide resolved
src/Build/Logging/FancyLogger/FancyLogger.cs Outdated Show resolved Hide resolved
src/Build/Logging/FancyLogger/FancyLoggerBuffer.cs Outdated Show resolved Hide resolved
@edvilme edvilme added merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. and removed merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. labels Jan 24, 2023
@edvilme edvilme requested a review from rainersigwald January 25, 2023 18:00
@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jan 25, 2023
@rainersigwald
Copy link
Member

@edvilme can you please merge main after we land the 17.5 merge into main (I think doing those together will minimize merge conflicts).

@rainersigwald rainersigwald removed the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jan 26, 2023
@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jan 30, 2023
 Conflicts:
	src/MSBuild/LiveLogger/LiveLogger.cs
	src/MSBuild/LiveLogger/TerminalBuffer.cs
Conflicts:
	src/MSBuild/LiveLogger/ANSIBuilder.cs
	src/MSBuild/LiveLogger/LiveLogger.cs
	src/MSBuild/LiveLogger/MessageNode.cs
	src/MSBuild/LiveLogger/ProjectNode.cs
@rainersigwald rainersigwald removed the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Feb 1, 2023
@rainersigwald rainersigwald marked this pull request as draft February 1, 2023 17:02
@rainersigwald
Copy link
Member

I merged #8314 into this speculatively and it's looking good but blocked on that, so drafted for now.

@rainersigwald rainersigwald marked this pull request as ready for review February 6, 2023 15:07
@rainersigwald
Copy link
Member

Undrafted since #8314 went in. Still LGTM to me, so marking merge-when.

@rainersigwald rainersigwald added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Feb 6, 2023
@JaynieBai JaynieBai merged commit 76215a0 into dotnet:main Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants