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

Review GHA for i18n consistency check #551

Merged
merged 13 commits into from
Aug 29, 2023

Conversation

spier
Copy link
Member

@spier spier commented Jun 20, 2023

This PR is making minor cosmetic modifications to the new GHA that checks the consistency of the translations.
See file: .github/workflows/i18n-consistency-checker.yaml

Main purpose of this PR is for me to understand what the GHA does, and to allow me to ask question to @yuhattor (who created this GHA). That way we have some written documentation that others can refer to later.

@spier spier changed the title review GHA for i18n consistency check Review GHA for i18n consistency check Jun 20, 2023
@spier spier requested a review from yuhattor June 20, 2023 13:00
Copy link
Member Author

@spier spier left a comment

Choose a reason for hiding this comment

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

@yuhattor left a couple of questions inline in this PR.

Note that I only read this script once, so I only have a rudimentary idea so far about how it works.

.github/workflows/i18n-consistency-checker.yaml Outdated Show resolved Hide resolved
@@ -79,6 +80,7 @@ jobs:
fi
done
- name: Create Issue
if: ${{ <expression> }}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not working as is.

However i was wondering if here we could test for the existence of issue.md and only run this entire step if that file exists?

Copy link
Member

Choose a reason for hiding this comment

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

That implementation should be in place below, but it doesn't seem to be working.
If there were a similar issue, it would update the new edits as a comment to that issue, so that a new issue would not be created.

Also, since the script is doing the checking for the issue's existence here, there should be an Action in the previous section to make that determination. The process would be like splitting the existing code.

            if expr "$existing_issue_id" : '^[0-9]*$' >/dev/null; then
              gh issue comment "$existing_issue_id" -F issue.md -R $GITHUB_REPOSITORY
            else
              gh issue create -t "$issue_title" -F issue.md -R $GITHUB_REPOSITORY -l documentation
            fi

@spier spier marked this pull request as ready for review June 20, 2023 13:18
@spier spier added Type - Maintenance / Cleanup Maintaining / cleaning the repo is the main focus of this issue / PR Type - Translation Translating patterns into other languages labels Jun 20, 2023
@yuhattor
Copy link
Member

Sorry for being late to reply. I'm starting to review now! Thank you for your PR!

…y again

* Refactor code

* FIx bugs

* Update token

* Update scripts

* Small fix
@yuhattor
Copy link
Member

yuhattor commented Aug 26, 2023

@spier Please check my update on this

So far, it has worked properly.
I also fixed a little script mess.

  • Specifically, to describe it in heredoc.
  • Now it also skips validated, but I changed it to include it when it loops.
  • Fixed Issue creation so that if there is an existing one, it is appended and not a new one is created
    • ↑ use :jp:expression for emoji instead of Emoji unicode

Here's sample
https://github.com/yuhattor/InnerSourcePatterns-Japanese/issues/10

Copy link
Member Author

@spier spier left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this @yuhattor.

I think I found (and fixed) some bugs.
Also added some more documentation.

.github/workflows/i18n-consistency-checker.yaml Outdated Show resolved Hide resolved
.github/workflows/i18n-consistency-checker.yaml Outdated Show resolved Hide resolved
# Get the translated file name and check if it exists
i18n_filename=$(echo "$file" | sed 's/patterns\/2-structured/translation\/${{matrix.language}}\/patterns/g')
# Declare the flags
declare -A flags=( ["ja"]=":jp: Japanese" ["zh"]=":cn: Chinese")
Copy link
Member Author

Choose a reason for hiding this comment

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

Where to lookup the icons to use here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@yuhattor this is the only question remaining that I think we should add to the documentation.

If you could share where to look up the strings like :jp:, then I can add that to the documentation.
I might do when working on #582.

.github/workflows/i18n-consistency-checker.yaml Outdated Show resolved Hide resolved
content_header=$(echo "$(cat "$file")" | grep -E '^title+' | sort -r | head -n1)

if [[ $((original_updated_at - i18n_content_updated_at)) -ge 1 ]]; then
content_header=$(grep -E '^title+' "$file" | sort -r | head -n1)
Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't look like the title extract works right now

However let's leave this in here, as we may be adding a frontmatter to all patterns anyways, at which point this should work :)

Copy link
Member

Choose a reason for hiding this comment

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

No wonder this thing doesn't work.
I was getting the title from the "title: foobar" meta info to match hugo's md style... But this time it's gitbook, so I need to extract from ## Title.
I didn't realize this when I ported...

I've changed it to use awk to extract it, so it should work now!

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Very pretty!

@yuhattor
Copy link
Member

Thank you for the addition, will comment on it soon...!

yuhattor and others added 2 commits August 29, 2023 17:28
LGTM! thanks

Co-authored-by: Sebastian Spier <github@spier.hu>
Co-authored-by: Sebastian Spier <github@spier.hu>
@spier
Copy link
Member Author

spier commented Aug 29, 2023

Thank you for working on this @yuhattor!

Please approve the PR when it looks good to go from your side.
You can also merge it yourself if you like :) (assuming that that is possible)

@yuhattor
Copy link
Member

Thank you! After some aditions... Now I confirmed that it works properly.
I will merge this now.

Thank you @spier for the contribution!!

@yuhattor yuhattor merged commit 99a58da into main Aug 29, 2023
11 checks passed
@yuhattor yuhattor deleted the review-GHA-for-i18n-consistency-check branch August 29, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type - Maintenance / Cleanup Maintaining / cleaning the 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.

None yet

2 participants