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

I18n check: fix empty comment #586

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
Open

Conversation

spier
Copy link
Member

@spier spier commented Sep 1, 2023

I noticed a bug in .github/workflows/i18n-consistency-checker.yaml.
When it runs but doesn't find any issues, it will still create a comment to the existing GitHub issue.
Example: #583 (comment)

This happened because the script wrote a header to issue.md, and later one the existence of the issue.md file was used to confirm whether an issue/comment should be created or not. As the header always exists, this was always true.

I fixes this by breaking up the logic into two main blocks:
a) check for inconsistencies between en and translation
b) if inconsistencies were found, create issue/comment

I am sure that my solution could be greatly improved with some more shell scripting knowledge.
Personally I am not super happy with my introduction of yet another file (issue-full.md). This could have likely been done differently as well.

Looking forward to any input.

@spier spier added ⚙️ Type - Meta Improving how we collaborate in this repo is the main focus of this issue / PR Type - Translation Translating patterns into other languages labels Sep 1, 2023
if: steps.check_files.outputs.files_exists == 'true'
run: |
# Heredoc for issue header
cat <<- EOM > issue-full.md
Copy link
Member Author

@spier spier Sep 1, 2023

Choose a reason for hiding this comment

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

Rather than introducing a new file, we could also try to prepend the header to the content of issue.md.

Logic in pseudo code:

issue.md = <header string> + issue.md (generated in previous steps)

Just not sure how to do this in a shell script :)

@@ -72,18 +61,41 @@ jobs:
EOM
fi
done

- name: Check issue.md existence
Copy link
Member Author

@spier spier Sep 1, 2023

Choose a reason for hiding this comment

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

Not sure if a GHA is needed to check for file existence.

Maybe this can be done directly with a GHA expression? Could not find the syntax for that though.
Also assumed that the file-existence-action probably was created for a reason (i.e. that it was not possible differently).

Copy link
Member

Choose a reason for hiding this comment

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

This implementation was set up in the past when we needed to do conditional calls on the GitHub Actions side, but as far as the current implementation is concerned, it only does the work when True, so I don't think we need this.

Copy link
Member

@yuhattor yuhattor Dec 6, 2023

Choose a reason for hiding this comment

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

Cross post #636 here
I edited this. Please merge this 🙏
I think my change is what you meant here. I did not push directly because I am not a bit confident about it, but I used PR!

@spier
Copy link
Member Author

spier commented Sep 1, 2023

@yuhattor I left this in draft because I think the code can be improved.

Also I found it a bit hard to test it end-to-end. I did some basic simulations but cannot say that I am 100% confident that this works as expected.

@spier spier requested a review from yuhattor September 1, 2023 10:52
@spier spier marked this pull request as ready for review November 18, 2023 21:32
@spier
Copy link
Member Author

spier commented Nov 18, 2023

@yuhattor would be great if you could help me check this PR.

@yuhattor
Copy link
Member

yuhattor commented Dec 6, 2023

@spier
Thank you for the PR and sorry for the delay!!
I haven't looked at this for a long time, and I've forgotten a bit about the implementation of this codebase.
I am forking it and checking it out. Give me a little time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚙️ Type - Meta Improving how we collaborate in this repo is the main focus of this issue / PR Type - Translation Translating patterns into other languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants