-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
removes arbitrary whitespace before unicode char/emojis #2680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
b7deeaf
to
d66affa
Compare
I thought this random indentation is intentional, but this is already very old code before I changed anything here. I don't think this is an area I would decide any direction.I'm not an fan of the emoji usage in act, but I might should have helped panekj more in his PR to remove the emojis a looooong time ago. This PR adds two additional emojis, not a fan of this |
whether to leave them in or remove them can we agree that it's currently inconsistent throughout the code base and should have something done. at the very least can we get this fix in so the beginning of messages are atleast more aligned then they currently are? |
I'm fine with merging this if someone is bothered by the space count. For reference this is the PR that was supposed to fix logging: #928 |
i also thought maybe there was some form of intentionality to it but i haven't been able to decipher anything, seems random. Maybe just my poor reading comprehension but it's more distracting than anything imo |
This is ok for me, but I only approve if necessary for merging once the PR gets one counting approval from someone else. Or if emoji up vote of 10+ happens for the original post, I cannot merge either way on my own. |
FYI changing these tests to pass is required as well
Please Ignore the TestImageExistsLocally Test, that might only need a rerun |
@ChristopherHX this should be fixed now, looks like it was just a consequence of 3x space being used in the test string. For future ref; what's the stance on using either a ~/.config/act/config.toml (or whatever file format) or looking for a environment variable (e.g. ACT_PLAIN_OUTPUT=true) to disable emojis? This would dictate what a purposed (better?) solution would look like for a refactor of logger to handle emojis |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2680 +/- ##
===========================================
+ Coverage 61.56% 74.41% +12.85%
===========================================
Files 53 72 +19
Lines 9002 11062 +2060
===========================================
+ Hits 5542 8232 +2690
+ Misses 3020 2193 -827
- Partials 440 637 +197 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
there's inconsistent usage of whitespace before and after emoji usage in log messages making the output excessively messy and messages misaligned.
this a bandaid fix as I could prob refactor the logger to use a standard convention so whitespace isn't hardcoded but for the time being this would be a welcome partial fix
Before fix output example:

After fix output example:
