ThePrez / ServiceCommander-IBMi

Service Commander for IBM i
Apache License 2.0
40 stars 12 forks source link

Feature/config color scheme #195

Closed ajshedivy closed 1 year ago

ajshedivy commented 1 year ago

Explain the reasoning for this pull request. For instance, is it for a new feature, bug fix, code style/cleanup, or something else? If fixing an open issue, please link to it here.

solution for suggestion in #190 for configuring terminal colors

Any additional comments/context?

To enable custom terminal colors, head to /QOpenSys/etc/sc/conf and open scrc with your favorite text editor. Add the following argument with a list of contexts and colors to change.

# Terminal Color defaults used in sc:
#   RUNNING: GREEN
#   NOT_RUNNING: PURPLE
#   INFO: CYAN
#   WARNING: YELLOW
#   ERROR: BRIGHT_RED
#   PLAIN: WHITE
#   STATUS: BLUE
#
# To override these colors, uncomment the following and
# add comma separated list of contexts and colors to change:
#
# --color-scheme=[CONTEXT:COLOR], [CONTEXT:COLOR]

--color-scheme=NOT_RUNNING:RED, RUNNING:CYAN

image

chrjorgensen commented 1 year ago

@ajshedivy Thanks for the PR!

One comment: You have too many commits not relevant to the feature for configuring terminal colors. I think you have based your feature branch on a branch other than the main branch. Please isolate the commits for terminal color, e.g. create a new feature branch only including commit https://github.com/ThePrez/ServiceCommander-IBMi/pull/195/commits/a820b5b4a6fdd351ce808f486fcbd1ebeb38d9a5 and https://github.com/ThePrez/ServiceCommander-IBMi/pull/195/commits/f69b1ad993c74d947a4401591392bd21a12e041f.

ajshedivy commented 1 year ago

Oops! I'll go ahead and fix that

chrjorgensen commented 1 year ago

@ajshedivy I'll review your upcoming fix tomorrow (I'm in CET) - or @ThePrez could do it if available?

ajshedivy commented 1 year ago

Not sure what the best approach is here, I can create a new PR with the correct commits on a new branch.

chrjorgensen commented 1 year ago

Yes, create a new branch - the name is not that important, it will only live until approved and merged into main.

ajshedivy commented 1 year ago

Okay, let's move discussion to #196, I think that should look better.

chrjorgensen commented 1 year ago

I'll review it tomorrow. Thanks!