Skip to content

fix: remove space after prefix from root error#1047

Merged
craicoverflow merged 3 commits intomainfrom
emoji-fix
Sep 8, 2021
Merged

fix: remove space after prefix from root error#1047
craicoverflow merged 3 commits intomainfrom
emoji-fix

Conversation

@craicoverflow
Copy link
Contributor

@craicoverflow craicoverflow commented Sep 8, 2021

Follow up to the great work done in #1046

Three things are covered here:

  • Renamed methods
  • Removed the extra space between the Error prefix and message
  • Detect if an emoji is used, and if so it capitalizes the first character if the message.

Verification Steps

Windows:

$ rhoas kafka topic list
Error: not logged in to identity.api.openshift.com. 

Linux:

$ rhoas kafka topic list
❌ Not logged in to identity.api.openshift.com.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation change
  • Other (please specify)

Checklist

  • Documentation added for the feature
  • CI and all relevant tests are passing
  • Code Review completed
  • Verified independently by reviewer


if !ok {
opts.Logger.Info(icon.Success(), opts.localizer.MustLocalize("login.log.info.loginSuccessNoUsername"))
opts.Logger.Info(icon.SuccessPrefix(), opts.localizer.MustLocalize("login.log.info.loginSuccessNoUsername"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just idea: opts.Logger.Success

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 might make things nicer actually, though I do like keeping the Logger lean and focused on log-levels only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

our logger is not logger per se. It is an stderr printer. Having more high-level methods will be good idea for future issue.

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, but the stderr printer is an implementation of a higher-level Logger interface, which could be implemented in different contexts. Though I do not see it being used in ways other than to stderr

@craicoverflow craicoverflow changed the title fix: remove prefix from root error fix: remove space after prefix from root error Sep 8, 2021
@craicoverflow craicoverflow merged commit 957c48d into main Sep 8, 2021
@craicoverflow craicoverflow deleted the emoji-fix branch September 8, 2021 09:24
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.

2 participants