Skip to content

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

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

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

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