Enable logging instance properties in default text loggers#5414
Enable logging instance properties in default text loggers#5414rainersigwald merged 62 commits intodotnet:masterfrom
Conversation
…a message. Note that this was added as a parameter because this class does not have access to the datastructures holding the target framework mappings.
…t framework and to print the target framework along with an error or warning message
…al errors and warning and to separate the error/warning summary by target framework
…t Framework information being printed on the console
…r/warning summary display to properly show.
… parameters of public facing methods
grammar fix Co-authored-by: Forgind <Forgind@users.noreply.github.com>
…y name. Replaced a forEach loop with a TryGet() for time improvement.
…tEventMessage. Increased efficiency by using TryGetValue and TryAdd instead of using containsKey
…this was not super helpful and could have potential issues with people's regex expressions
…ers and their values
…n warning/error messages based on a customizable item defined in a project.
…erties are printed has been moved
…nt out would not have errors
… Dictionary. TryAdd() is not available in net472 so added if/else based on framework
…s to ensure that regex expressions can easily parse the file path but also makes it a bit easier for the user to read
Forgind
left a comment
Forgind
left a comment
There was a problem hiding this comment.
Strong start! I don't understand why you made some of the changes, but I'll look again later to see if it makes sense then.
| } | ||
| } | ||
| #endif No newline at end of file | ||
| #endif |
There was a problem hiding this comment.
undo changes like this or extra spaces, please
…is now a key/value pair that is printed as is
|
Wouldn't it be better if the engine computed this string, rather than a specific logger? If loggers are responsible for computing it, then everybody who writes a logger has to duplicate this logic. If the engine computes it, then it's in a single place.
Another benefit of putting it in the Taking it even further, it could even be a property of |
| if (string.Equals((string)item.Key, "LogOutputProperties", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| // Add the item value to the string to be printed in error/warning messages | ||
| logOutputProperties.Append(" ").Append(itemVal.ItemSpec); |
There was a problem hiding this comment.
It might be good to allow configuring the separator via a property (like MSBuildProjectConfigurationDescriptionSeparator). Default to space if none is set.
There was a problem hiding this comment.
That makes parsing harder so I think I'm against it.
This is a nice design, I think.
Maybe, but right now I think I prefer leaving it at the logging level (in the event). |
|
Talked this over with @laurenprinn and others. I like pushing it to the LoggingService, but that doesn't solve the whole problem, because it's only easy to do that for the We could add it to a deep base class like So let's go with this implementation for now and we can potentially improve it further in the future. |
Start at a more e2e test
rainersigwald
left a comment
rainersigwald
left a comment
There was a problem hiding this comment.
Many nits, looking great overall!
| int projectContextId = e.BuildEventContext.ProjectContextId; | ||
|
|
||
| // Create the value to be added to the propertyOutputMap | ||
| StringBuilder msBuildProjectConfigurationDescription = new StringBuilder(); |
There was a problem hiding this comment.
Hmm, works on my machine™:
| StringBuilder msBuildProjectConfigurationDescription = new StringBuilder(); | |
| using var msBuildProjectConfigurationDescription = new ReuseableStringBuilder(); |
rainersigwald
changed the title
ghost
added this to the
6 participants
Edits console logger such that various properties can be output in an error or warning message. The properties are determined by an item "outputProperties" defined by the project. Ex:
Program.cs(14,13): error CS0103: The name 'Consol' does not exist in the current context [C:\Users\t-laprin\source\repos\tester_app\tester_app\tester_app.csproj SchemaVersion:2.0 TargetFramework:netcoreapp3.0 ]
I don't have write permission to Link an issue to this, but this would be the issue it should link to: #3015