-
Notifications
You must be signed in to change notification settings - Fork 193
animals as text instead of emoji #4184
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4184 +/- ##
==========================================
- Coverage 49.55% 48.65% -0.90%
==========================================
Files 452 482 +30
Lines 44849 46969 +2120
==========================================
+ Hits 22225 22853 +628
- Misses 20513 21954 +1441
- Partials 2111 2162 +51
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
If these are truly just concurrency identifiers, can't we just use "Thread 01-Thread 12", or "Job 01-Job 12" (or with zero-based numbering if you prefer)? |
i like the animal names but if its a "hard no" then ill replace w/ ints let me know @jmoroski . In all seriousness, here is my case for keeping the "animal spirit" alive:
either way is fine tho :)just lmk if you'd strongly prefer int IDs, ill do:
so theyr unique and searchable... |
If this was internal only, I'd tell you to use whatever tasteful collection of animals you choose. But I believe this builder plugin is what we currently tell people outside of our team (and possibly organization) to use to scaffold and build plugins. So IDs is the less fun, but better choice. I'm fine with your T1, T2, ...Tn option. |
Ack yup t1 is traceable no problem will fix it to that |
fixed the emjois to T1-9 ! |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nit, if you do 0 padding (T01, T02...T12) there will be a known fixed width for log parsers.
Take it or leave it, this is approved.
Thanks @jayunit100 . Looking specifically at the output I think we have some inconsistencies. This is what I see:
I realize this is nit-picky, but since we have our hands in there, might as well...
Just some suggestions... |
Now im starting to think Maybe I should role this in w a broader klog replacement :) |
I echo @marckhouzam 's thoughts. The padding for better alignment, using matching ids for the "starting.." log line, as well as tweaking the $ looks straightforward enough to add. |
just saw this, fixed mark's suggestions... but im also going to look into klog more broadly as well today. |
fixes #4183
old output:
new output: