Skip to content
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

Add a check for zero-byte files. #5374

Closed
wants to merge 15 commits into from

Conversation

desrosj
Copy link
Contributor

@desrosj desrosj commented Oct 3, 2023

Trac ticket: https://core.trac.wordpress.org/ticket/59527


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

show-progress: ${{ runner.debug == '1' && 'true' || 'false' }}

- name: Find zero-byte files
run: find src/wp-content/themes/${{ matrix.theme }} -size 0
Copy link
Contributor

@costdev costdev Oct 3, 2023

Choose a reason for hiding this comment

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

Something like this may work:

if [[ "" != $(find src/wp-content/themes/${{ matrix.theme }} -size 0) ]]; then exit 1; fi

Not sure if that translates to the workflow properly, but you get the gist 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! I didn't see this before making 7f5fb21. What do you think about that approach? I think it reads a lot easier. But not opposed to something like above either.

Copy link
Contributor

@costdev costdev Oct 4, 2023

Choose a reason for hiding this comment

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

The other approach works but I'm not sure that it'll be clear that the diff check is part of, and required for, the empty file checks.

This also works in bash and may work here:

- name: Make sure there are no empty files.
- run: [[ ! $(find src/wp-content/themes/${{ matrix.theme }} -empty) ]]

Do you think that's readable enough or not quite?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I've added this, but it doesn't seem to be flagging the empty images (I haven't synced my PR branch since that was fixed in trunk).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it should be producing a 1 exit code. Strange. Unless a readable oneliner comes to mind, the diff check makes most sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strange! I just tried it again and it did work. 🤷‍♂️

@desrosj
Copy link
Contributor Author

desrosj commented Oct 6, 2023

@desrosj desrosj closed this Oct 6, 2023
@desrosj desrosj deleted the add/0-byte-file-check branch October 6, 2023 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants