Conversation
zadjii-msft
left a comment
zadjii-msft
left a comment
There was a problem hiding this comment.
okay big-picture feedback:
I feel like exposing GenerateID() via the ActionAndArgs projection feels like an ick. The IDs probably shouldn't be something that we ever expose externally, right?
We do have the ShortcutAction Action on the args. Granted, that's a enum value, and idk if there's a trivial way for us to convert that enum value label to a string in the TerminalApp library. But I'd rather expose something like ShortcutActionToString(ShortcutAction a), and call ShortcutActionToString(actionAndArgs.Action()), rather than expose the ID.
(i also know this is for a relatively trivial tracelogging request that we probably don't want to overengineer for)
Thoughts?
|
Do we need to log specific actions? Can we just log action types? |
|
That's a fair point. If we don't care about the string names of the actions themselves, then we can just write out the We'd have to be careful in the future to make sure to append-only to that list, but that doesn't seem like the end of the world. |
microsoft-github-policy-service
Bot
requested review from
DHowett,
PankajBhojwani,
lhecker and
zadjii-msft
microsoft-github-policy-service
Bot
removed
the
Needs-Second
Some simple logic to report whenever an action has successfully occurred (and what ShortcutAction was used). Note, there will be some false positives here from startup. I noticed we get a `newTab` on launch. This is probably a result of restoring the window layout of the previous session since we're using ActionAndArgs for that. (cherry picked from commit 56cfb77) Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgSCpCo Service-Version: 1.21
4 participants
Some simple logic to report whenever an action has successfully occurred (and what ShortcutAction was used).
Note, there will be some false positives here from startup. I noticed we get a
newTabon launch. This is probably a result of restoring the window layout of the previous session since we're using ActionAndArgs for that.