act icon indicating copy to clipboard operation
act copied to clipboard

removes arbitrary whitespace before unicode char/emojis

Open exastone opened this issue 10 months ago • 8 comments

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: act-whitespace-example-before

After fix output example: act-whitespace-example-after

exastone avatar Feb 22 '25 19:02 exastone

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

ChristopherHX avatar Feb 23 '25 18:02 ChristopherHX

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?

exastone avatar Feb 23 '25 18:02 exastone

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: https://github.com/nektos/act/pull/928

panekj avatar Feb 23 '25 18:02 panekj

I thought this random indentation is intentional

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

exastone avatar Feb 23 '25 19:02 exastone

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.

ChristopherHX avatar Feb 23 '25 19:02 ChristopherHX

FYI changing these tests to pass is required as well

✖  pkg/runner (5m28.277s) (coverage: 57.0% of statements in ./...)

=== Failed

=== FAIL: pkg/runner TestAddmaskUsemask (0.00s)
    command_test.go:174: 
        	Error Trace:	/home/runner/work/act/act/pkg/runner/command_test.go:174
        	Error:      	Not equal: 
        	            	expected: "[testjob]⚙  ***\n[testjob]⚙  ::set-output:: = token=***\n"
        	            	actual  : "[testjob] ⚙  ***\n[testjob] ⚙  ::set-output:: = token=***\n"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,3 +1,3 @@
        	            	-[testjob]⚙  ***
        	            	-[testjob]⚙  ::set-output:: = token=***
        	            	+[testjob] ⚙  ***
        	            	+[testjob] ⚙  ::set-output:: = token=***
        	            	 
        	Test:       	TestAddmaskUsemask

DONE 1153 tests, 2 failures in 379.272s
exit status 1

Please Ignore the TestImageExistsLocally Test, that might only need a rerun

ChristopherHX avatar Feb 23 '25 21:02 ChristopherHX

@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

exastone avatar Feb 24 '25 14:02 exastone

Codecov Report

Attention: Patch coverage is 89.65517% with 3 lines in your changes missing coverage. Please review.

Project coverage is 74.41%. Comparing base (5a80a04) to head (0810d44). Report is 187 commits behind head on master.

Files with missing lines Patch % Lines
pkg/runner/command.go 92.85% 1 Missing :warning:
pkg/runner/run_context.go 75.00% 1 Missing :warning:
pkg/runner/step.go 80.00% 1 Missing :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Feb 24 '25 15:02 codecov[bot]