Skip to content

Conversation

@exastone
Copy link

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 exastone requested a review from a team as a code owner February 22, 2025 19:53
@exastone exastone changed the title removes arbitrary whites before unicode char/emojis removes arbitrary whitespace before unicode char/emojis Feb 22, 2025
@exastone exastone force-pushed the exastone/fix-emoji-whitespace branch from b7deeaf to d66affa Compare February 22, 2025 20:13
@ChristopherHX
Copy link
Contributor

ChristopherHX commented Feb 23, 2025

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

@exastone
Copy link
Author

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?

@panekj
Copy link
Contributor

panekj commented Feb 23, 2025

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

@exastone
Copy link
Author

exastone commented Feb 23, 2025

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

@ChristopherHX
Copy link
Contributor

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.

@mergify mergify bot added the needs-work Extra attention is needed label Feb 23, 2025
@ChristopherHX
Copy link
Contributor

ChristopherHX commented Feb 23, 2025

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

@exastone
Copy link
Author

@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
Copy link

codecov bot commented Feb 24, 2025

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 ⚠️
pkg/runner/run_context.go 75.00% 1 Missing ⚠️
pkg/runner/step.go 80.00% 1 Missing ⚠️
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.
📢 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.

@mergify mergify bot removed the needs-work Extra attention is needed label Feb 24, 2025
@mergify mergify bot requested a review from a team February 24, 2025 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants