Prefer use of interpolated strings in PresentationCore #8616
Conversation
ghost
assigned 
ghost
requested review from
| #if DEBUG | ||
| if (_traceSwitch.Enabled) | ||
| System.Diagnostics.Trace.TraceInformation( | ||
| DateTime.Now.ToLongTimeString() + " " + DateTime.Now.Millisecond + " " + | ||
| Environment.CurrentManagedThreadId + | ||
| ": SiteOfOriginContainer: returning site of origin " + siteOfOrigin); | ||
| $"{DateTime.Now:T} {DateTime.Now.Millisecond} {Environment.CurrentManagedThreadId}: SiteOfOriginContainer: returning site of origin {siteOfOrigin}"); |
There was a problem hiding this comment.
A follow-up to this is to introduce a Trace method with a custom interpolated string handler that encapsulate all this logic. It would clean-up the code quite a lot
There was a problem hiding this comment.
@halgab Having an encapsulated function would be beneficial for maintaining clean code. Could you please provide the function's arguments and structure that you are considering?
We are planning to add this PR for this month test-pass. So, can you resolve the merge conflicts for this PR?
There was a problem hiding this comment.
I've added a draft PR based on this one here: #8702. Its last commit is what I have in mind. It's based on similar changes I contributed in the winforms repo.
halgab
force-pushed
the
interpolated-string-core
branch
from
429535a
to
4dd05ab
Compare
halgab
changed the title
halgab
force-pushed
the
interpolated-string-core
branch
from
4dd05ab
to
4ea7631
Compare
There was a problem hiding this comment.
@halgab A DRT(test) is failing on this PR: DrtColorAPI
You can find this test on our public repo: https://github.com/dotnet/wpf-test
I think so the test is failing due to some missing elements in this file after change.
There was a problem hiding this comment.
After having another read, the difference between the current version and mine is that when nativeColorValue is empty, the current version still appends a separator value, which is not the case with my version. I feel like my version is more correct.
Tell me if you prefer that I maintain the current behaviour.
There was a problem hiding this comment.
Your changes are functionally different. Floats were formatted using format previously but are now always formatted using R. Also, the loop in your changes always prepends the separator instead of prepending the separator only when between items: ",A,B,C" vs "A,B,C". Also, there should be a space here between {uriString} and {scRgbColor.a:R}, it was there before your changes.
I would recommend testing your changes because by looking at the code I don't see how your changes returns the same result as the existing code.
There was a problem hiding this comment.
Those are valid points. I'll rollback the functional changes. I'd like to keep the constant format though, as format is always set to R before this loop, and that's what makes it possible to use interpolated strings. What do you think?
halgab
force-pushed
the
interpolated-string-core
branch
from
b63dd61
to
e7de550
Compare
Follow-up to #8519
Description
We replace as many manual string concatenations and string.Format with interpolated strings. This saves allocations in many instances, and makes for a more readable code.
Customer Impact
Less allocations
Regression
No
Testing
CI
Risk
Low. Most changes are automated using the IDE
Microsoft Reviewers: Open in CodeFlow