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] -> LiveLogger #8356

Merged
merged 24 commits into from
Feb 1, 2023
Merged

Conversation

edvilme
Copy link
Member

@edvilme edvilme commented Jan 26, 2023

Fixes #

Context

FancyLogger LiveLogger is the new console logger for MSBuild. The name LiveLogger better describes its functionality.

Changes Made

  • Renamed FancyLogger and helper classes to LiveLogger
  • Added /ll and /livelogger command line switches to access the new LiveLogger
  • Added $MSBUILDLIVELOGGER environment variable to acces the new LiveLogger

Testing

Notes

All references to FancyLogger have been replaced. However, the /flg and /fancylogger command line switches remain.

@edvilme edvilme changed the title [FancyLogger] -> ~Fancy~ Live [FancyLogger] -> LiveLogger Jan 26, 2023
Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

It looks like GitHub is correct here in that the merge put the FancyLogger files back. Specifically, check out src\MSBuild\FancyLogger*.

I seem to recall moving to that directory was intentional, so I'm guessing we should just move these over there and delete the FancyLogger files.

@rainersigwald
Copy link
Member

I'm making a couple of refactors to this.

@rainersigwald
Copy link
Member

Actually no, we'll need to go back further.

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Ok should be ready for review now. I did basically the same things but simplified the names, and changed some of the cli-argument-parsing stuff.

Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

I kinda prefer the more descriptive "LiveLoggerProjectNode"-style names unless we're actually likely to reuse any of them, and honestly, if we reuse something, and it's internal, we can rename it then. I'm not blocking on that, but that's my vote.

I am blocking on the src\MSBuild\FancyLogger\FancyLogger.cs --> src\MSBuild\LiveLogger\FancyLogger.cs part. The file shouldn't be named FancyLogger.cs.

@rainersigwald
Copy link
Member

I kinda prefer the more descriptive "LiveLoggerProjectNode"-style names unless we're actually likely to reuse any of them, and honestly, if we reuse something, and it's internal, we can rename it then. I'm not blocking on that, but that's my vote.

The disambiguation is in the namespace and IMO does not serve a purpose in the class name.

I am blocking on the src\MSBuild\FancyLogger\FancyLogger.cs --> src\MSBuild\LiveLogger\FancyLogger.cs part. The file shouldn't be named FancyLogger.cs.

🤦🏻

@Forgind
Copy link
Member

Forgind commented Jan 30, 2023

This is the experience I don't like:
image

I often search for things with ctrl-t, and if I look for things related to Project, and ProjectNode shows up, I'd assume that's related until I look to the right and notice it's in a weird path. Just trying to save myself some headache 🙂

(Granted, it happens to be far enough down currently that I wouldn't notice it, but it's still there—and on top if I search for ProjectN.)

The last three places I see FancyLogger are comments (which you may not care about), documentation\fancylogger\Opt-In-Mechanism.md (which I think should be updated), and https://github.com/dotnet/msbuild/pull/8356/files#diff-a0455d9446db624e81fedafd35e83bb7743f56419a2362a1a05dbc81dc84548bL11 along with its references.

@edvilme
Copy link
Member Author

edvilme commented Jan 30, 2023

Just updated the documentation to reflect the new name. I agree it should be updated overall, will look into that in the future ;)

The last three places I see FancyLogger are comments (which you may not care about), documentation\fancylogger\Opt-In-Mechanism.md (which I think should be updated), and https://github.com/dotnet/msbuild/pull/8356/files#diff-a0455d9446db624e81fedafd35e83bb7743f56419a2362a1a05dbc81dc84548bL11 along with its references.

@Forgind
Copy link
Member

Forgind commented Jan 30, 2023

Line 6 in the documentation still mentions the FancyLogger. Also, it looks like some random binary files got added?

@edvilme
Copy link
Member Author

edvilme commented Jan 30, 2023

My bad. Updated it :D
I am working from a Mac, so that's why the .DS_Store files are added. If it is okay, I will create a PR to include .DS_Store in the .gitignore

@Forgind
Copy link
Member

Forgind commented Jan 30, 2023

You're good 🙂

I'm fine with it being in the .gitignore; you can also disable it for yourself with:
defaults write com.apple.desktopservices DSDontWriteNetworkStores true

@edvilme
Copy link
Member Author

edvilme commented Jan 30, 2023

This is a life changer, thanks! I will add it later still for any other contributors using Mac.

@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 Jan 31, 2023
@Forgind Forgind 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 31, 2023
@Forgind
Copy link
Member

Forgind commented Jan 31, 2023

@rainersigwald, edvilme only addressed one part of #8356 (comment). This isn't ready to go in.

@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 31, 2023
@MichalPavlik MichalPavlik merged commit 7cfb36c into dotnet:main Feb 1, 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