search

microsoft / storage-as-a-service

MIT License
12 stars 9 forks source link

Replace log entries using string interpolation with direct arguments #63

Closed heathbm closed 2 years ago

heathbm commented 2 years ago

Implementation of #45

ghost commented 2 years ago

CLA assistant check
All CLA requirements met.

github-actions[bot] commented 2 years ago

Azure Static Web Apps: Your stage site is ready! Visit it here: https://white-pebble-03f47660f-63.eastus2.1.azurestaticapps.net

SvenAelterman commented 2 years ago

@heathbm Thank you for your contribution. It's being reviewed now.

One question, are the changes you made comprehensive to resolve the issue across the entire code base, or did you pick and choose specific classes?

heathbm commented 2 years ago

@heathbm Thank you for your contribution. It's being reviewed now.

One question, are the changes you made comprehensive to resolve the issue across the entire code base, or did you pick and choose specific classes?

@SvenAelterman These are all I could find across the code-base. I added another commit as I missed one. I can't find anymore now searching for *.Log*

SvenAelterman commented 2 years ago

@heathbm I think there are two instances where a message returned to the client/caller would not be formatted as expected. Can you address those? It's a bit unfortunate, but it seems like where won't be a way to avoid duplicating the strings.

github-actions[bot] commented 2 years ago

Azure Static Web Apps: Your stage site is ready! Visit it here: https://white-pebble-03f47660f-63.eastus2.1.azurestaticapps.net

heathbm commented 2 years ago

"Error while trying to query for the existence of container '{fileSystemName}' in account '{dlsClientAccountName}': '{exMessage}'."

@SvenAelterman string.Format() should help avoid duplicating strings. However, if you would not like to use it, I can update this to simply use interpolation for the result message.

SvenAelterman commented 2 years ago

@heathbm I don't have a problem with string.Format() but I am unaware that it works with "named" placeholders instead of indexed placeholders.

heathbm commented 2 years ago

@heathbm I don't have a problem with string.Format() but I am unaware that it works with "named" placeholders instead of indexed placeholders.

@SvenAelterman I updated the string to use indexed placeholders.

SvenAelterman commented 2 years ago

@heathbm I don't have a problem with string.Format() but I am unaware that it works with "named" placeholders instead of indexed placeholders.

@SvenAelterman I updated the string to use indexed placeholders.

My mistake, I just see the updated code now. I'll need to review to confirm that indexed placeholders are going to work for structured logging; I've never tried it or seen it.

heathbm commented 2 years ago

@heathbm I don't have a problem with string.Format() but I am unaware that it works with "named" placeholders instead of indexed placeholders.

@SvenAelterman I updated the string to use indexed placeholders.

My mistake, I just see the updated code now. I'll need to review to confirm that indexed placeholders are going to work for structured logging; I've never tried it or seen it.

@SvenAelterman Apologies, I tried it out and unfortunately it does not work so I added a commit to use structed logging and separately string interpolation for the result message.

SvenAelterman commented 2 years ago

@heathbm Thanks for sticking with it. Approved.

github-actions[bot] commented 2 years ago

Azure Static Web Apps: Your stage site is ready! Visit it here: https://white-pebble-03f47660f-63.eastus2.1.azurestaticapps.net