Skip to content

Conversation

@stephen-carter-at-sf
Copy link
Contributor

  • Added give-feedback message and url to each command's help description
  • Added give-feedback message and to end of the log file
  • Removed draft markings of few remaining messages.

Copy link
Contributor

@jshackell-sfdc jshackell-sfdc left a comment

Choose a reason for hiding this comment

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

Looks great! I have one little edit.

}

public stopListening(): void {
this.logWriter.writeToLog('\n' + getMessage(BundleName.Shared, 'log.give-us-feedback'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Is it okay for this to be a raw string without a log-level, source, etc? I know that conceptually it doesn't have those things, but I'm just unsure of whether logging something that lacks them could interfere with displaying or filtering logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's a good question. I had the same thought. I'm not sure if the goal is to make the logs machine readable or not. If so - then we'd need to adjust the formatting to be more deterministic since currently messages can already go to multiple lines and possibly corrupt the formatting.

I currently don't think we need the log to be machine readable - my guess is that it will always just be for human readability when diagnosing issues. So I think having a newline plus a plain string works here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

@stephen-carter-at-sf stephen-carter-at-sf merged commit a132990 into dev-5 Nov 15, 2024
9 checks passed
@stephen-carter-at-sf stephen-carter-at-sf deleted the sc/W-17100389 branch November 15, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants