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

feat: prevent blankline commits #3858

Merged
merged 3 commits into from Oct 3, 2023
Merged

feat: prevent blankline commits #3858

merged 3 commits into from Oct 3, 2023

Conversation

melsonic
Copy link
Contributor

@melsonic melsonic commented Oct 2, 2023

What does this PR do?

It prevents commits that contain only blank lines.

Related issues

Fixes # 3807

/claim #3807

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Enhancement (non-breaking small changes to existing functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Explanation of the changes

  • changed the pre-commit hook that checks if the staged changes only contain blank lines before committing the changes.

Emoji

✅ ✅ ✅

@melsonic melsonic requested a review from bigint as a code owner October 2, 2023 13:37
@vercel
Copy link

vercel bot commented Oct 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
prerender ✅ Ready (Inspect) Visit Preview Oct 3, 2023 5:40pm
web ✅ Ready (Inspect) Visit Preview Oct 3, 2023 5:40pm

@@ -1,5 +1,12 @@
#!/bin/sh
. "$(dirname "$0")/_/husky.sh"

if [ -z "$(git diff --cached --ignore-blank-lines --unified=0 | grep -E '^[+-]\s' | grep -vE '^[+-]\s*$')" ]; then
echo "Error: commit must contain new content."
exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

The commits with empty lines are generated by automated workflow scripts:

https://github.com/heyxyz/hey/blob/main/.github/workflows/i18n-extract.yml
https://github.com/heyxyz/hey/blob/main/.github/workflows/crowdin-download.yml

So if we just do a pre-commit hook, the scripts would fail, I assume?
I think that this issue needs to be solved by configuring the workflow scripts.

Copy link
Member

Choose a reason for hiding this comment

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

exactly we need to solve in crowdin config imo, they might have something there

Copy link
Contributor

@foolo foolo Oct 3, 2023

Choose a reason for hiding this comment

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

it seems like crowdin-download.yml adds extra newlines to the end of .po files, and i18n-extract.yml removes them again :)

#3807 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for these insights, will do the needful asap 👍

@@ -1,5 +1,12 @@
#!/bin/sh
. "$(dirname "$0")/_/husky.sh"

if [ -z "$(git diff --cached --ignore-blank-lines --unified=0 | grep -E '^[+-]\s' | grep -vE '^[+-]\s*$')" ]; then
echo "Error: commit must contain new content."
exit 1
Copy link
Member

Choose a reason for hiding this comment

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

exactly we need to solve in crowdin config imo, they might have something there

@vercel vercel bot temporarily deployed to Preview – prerender October 3, 2023 13:04 Inactive
@melsonic
Copy link
Contributor Author

melsonic commented Oct 3, 2023

Please review the changes @bigint

Comment on lines 32 to 35

- name: Remove last empty lines from translations
run: |
find '/**/locales/' -type f -name '*.po' -exec sed -i -e '${/^$/d;}' {} \;
Copy link
Member

Choose a reason for hiding this comment

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

have you tested this? if we add here how it will remove after the file is being pushed on previous step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think instead of creating a pull request during the Download from Crowdin ⬇️ step, we should be creating a pull request at the end.

Copy link
Member

Choose a reason for hiding this comment

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

Can you fix the workflow?

@vercel vercel bot temporarily deployed to Preview – prerender October 3, 2023 17:39 Inactive
@bigint bigint merged commit ad0a781 into heyxyz:main Oct 3, 2023
5 checks passed
@bigint
Copy link
Member

bigint commented Oct 3, 2023

The CI fails

image

I'm reverting this PR

@bigint
Copy link
Member

bigint commented Oct 3, 2023

@melsonic reopen this PR again 🙇🏼

@bigint
Copy link
Member

bigint commented Oct 3, 2023

note: our default standard in this repo to add new line to EOF, so it is expected to have blank line at the end

@TouchstoneTheDev
Copy link

is #3807 fixed ?

@bigint
Copy link
Member

bigint commented Oct 7, 2023

@TouchstoneTheGreat not yet!

@bigint
Copy link
Member

bigint commented Oct 7, 2023

we reverted the changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants