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

Use MSBuild's logging facilities instead of writing to console. #13896

Merged
merged 5 commits into from
Oct 14, 2022

Conversation

teo-tsirpanis
Copy link
Contributor

The repository's MSBuild tasks log messages directly to the console, which is more verbose than needed, cannot be made less verbose, and does not play well with MSBuild's logging facilities.

This PR changes the tasks to use the Lawful Good™ way of logging events. Also instead of implementing the Microsoft.Build.Framework.ITask interface, they now inherit the Microsoft.Build.Utilities.Task class, which is easier to use.

@T-Gro
Copy link
Member

T-Gro commented Oct 4, 2022

Hi, this is good staff.
Can I help a little and make changes to satisfy CI checks?

@teo-tsirpanis
Copy link
Contributor Author

Oh, didn't see the CI failures. Sure, go ahead, thanks.

T-Gro
T-Gro previously approved these changes Oct 12, 2022
@T-Gro T-Gro self-assigned this Oct 12, 2022
@T-Gro T-Gro requested a review from KevinRansom October 12, 2022 11:19
@T-Gro
Copy link
Member

T-Gro commented Oct 12, 2022

@KevinRansom :
Is this ok to merge from your perspecitve? Can you approve please?

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

It's a bit of unknown territory for me yet so my comments might be irrelevant a bit :)

Generally using the built-in logging mechanism instead of printfs everywhere is a right thing to do so I think this should get in - my comments are mostly about other changes here.

src/FSharp.Build/FSharpEmbedResXSource.fs Show resolved Hide resolved
src/FSharp.Build/FSharpEmbedResXSource.fs Show resolved Hide resolved
src/FSharp.Build/FSharpEmbedResourceText.fs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants