iScsc / iscsc.fr

The iScsc website, build with passion by wannabe devs 🔥
GNU General Public License v3.0
4 stars 12 forks source link

FEATURE: add tool functions for logging to `bump.sh` #67

Closed amtoine closed 1 year ago

amtoine commented 1 year ago

This PR

things to pay attention to

given the content of the various [.] in the script, i've come up with the following transition table token tool function color code color
! log_error 31 red
i log_ok 32 green
~ log_warning 33 yellow
? log_hint 34 blue
+ log_info 35 magenta
- log_normal 36 cyan

:point_right: i'm not sure about the exact names and the colors...

:pray: please tell me if any of them should change!!

amtoine commented 1 year ago

About the names/colors I would suggest: log_ok --> log_info log_info --> log_ok [-] is the real error I think thinking I just wrongly use [!] once or twice, could you replace every [!] (currently log_error) by [-] and rename log_normal --> log_error (using red then)

don't think it's crystal clear sorry about that!

i'm not 100% sure, but tell me if what i've tried below is fine :yum: see cc054bb^..2d337af composed of cc054bb and 2d337af

if that's what you meant, i'll push these two commits on the PR branch :wink:

ctmbl commented 1 year ago

if that's what you meant, i'll push these two commits on the PR branch

Actually almost! I'd just invert the colors of log_info and log_ok (to have [i]==info in purple and [+]==ok in green) but yours seems fine too.

amtoine commented 1 year ago

Actually almost! I'd just invert the colors of log_info and log_ok (to have [i]==info in purple and [+]==ok in green) but yours seems fine too.

like this? :smirk: see 83f045d

ctmbl commented 1 year ago

@amtoine looks perfect! I understand your workflow of pushing these changes on a test/* branch until you know they are validated but it can be a little confusing while testing (fetching the PR branch isn't enough I have to remember to fetch the amtoine test/* branch). A PR branch is a already a construction branch, no worries if you sometime have to revert commits or override some previous changes (like in this case), could you avoid doing this for the future review? :confused:

Apart from this I think your PR is ready to be merged :wink:

amtoine commented 1 year ago

I understand your workflow of pushing these changes on a test/* branch until you know they are validated but it can be a little confusing while testing (fetching the PR branch isn't enough I have to remember to fetch the amtoine test/* branch). A PR branch is a already a construction branch, no worries if you sometime have to revert commits or override some previous changes (like in this case), could you avoid doing this for the future review? confused

got it :ok_hand: i sometimes branch early and often, feel free to not fetch these branches and ask me to push on the PR branch directly :wink:

Apart from this I think your PR is ready to be merged wink

great :muscle: @atxr? :yum:

atxr commented 1 year ago

I understand your workflow of pushing these changes on a test/* branch until you know they are validated but it can be a little confusing while testing (fetching the PR branch isn't enough I have to remember to fetch the amtoine test/* branch). A PR branch is a already a construction branch, no worries if you sometime have to revert commits or override some previous changes (like in this case), could you avoid doing this for the future review? confused

got it :ok_hand: i sometimes branch early and often, feel free to not fetch these branches and ask me to push on the PR branch directly :wink:

Apart from this I think your PR is ready to be merged wink

great :muscle: @atxr? :yum:

I'll check within the week!

ctmbl commented 1 year ago

@amtoine I think the chnages we discussed are still on your test/* branch, could you rebase them onto this PR branch? :yum:

amtoine commented 1 year ago

@amtoine I think the chnages we discussed are still on your test/* branch, could you rebase them onto this PR branch? yum

woooopsie, yep :eyes:

amtoine commented 1 year ago

@ctmbl @atxr that should do it, sorry for the back-and-forth :grimacing: