Skip to content

chore(ci): remove redundant ${{ ... }} in if statements in workflows - #4517

Closed
programmer04 wants to merge 1 commit into
mainfrom
rm-redundant-parentheses
Closed

programmer04 wants to merge 1 commit into
mainfrom
rm-redundant-parentheses

Conversation

@programmer04

@programmer04 programmer04 commented Aug 16, 2023

Copy link
Copy Markdown
Member

What this PR does / why we need it:

When you use expressions in an if conditional, you may omit the ${{ ... }} expression syntax because GitHub Actions automatically evaluates the if conditional as an expression. Read more. I found a corner case where such omit is not applicable, e.g.

read more here.

Issue created in GH docs github/docs#27497

@programmer04 programmer04 added area/ci area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. labels Aug 16, 2023
@programmer04 programmer04 self-assigned this Aug 16, 2023
@programmer04
programmer04 requested a review from a team as a code owner August 16, 2023 09:29
@programmer04
programmer04 enabled auto-merge (squash) August 16, 2023 09:29
@programmer04
programmer04 force-pushed the rm-redundant-parentheses branch from 1740506 to b6770b6 Compare August 16, 2023 09:54

@randmonkey randmonkey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all the remaining ${{ }} s are necessary?

@codecov

codecov Bot commented Aug 16, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage has no change and project coverage change: -0.1% ⚠️

Comparison is base (c397107) 67.7% compared to head (b6770b6) 67.7%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4517     +/-   ##
=======================================
- Coverage   67.7%   67.7%   -0.1%     
=======================================
  Files        161     161             
  Lines      18845   18845             
=======================================
- Hits       12769   12761      -8     
- Misses      5315    5326     +11     
+ Partials     761     758      -3     

see 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@programmer04

Copy link
Copy Markdown
Member Author

I think so @randmonkey only in if statements (with a caveat) they can be omitted

@programmer04
programmer04 marked this pull request as draft August 16, 2023 11:02
auto-merge was automatically disabled August 16, 2023 11:02

Pull request was converted to draft

@pmalek

pmalek commented Aug 29, 2023

Copy link
Copy Markdown
Member

@programmer04 Are we still working on this 🤔 ?

@programmer04

programmer04 commented Aug 29, 2023

Copy link
Copy Markdown
Member Author

I put it on hold @pmalek because I discovered a quick that I wanted to clarify github/docs#27497. Now knowing that I lean towards putting ${{ }} always in if 😛

@programmer04

Copy link
Copy Markdown
Member Author

Since omitting doesn't work always (see official docs) I'm not longer advocating for adjusting it

@programmer04
programmer04 deleted the rm-redundant-parentheses branch September 6, 2023 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ci area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants