-
-
Notifications
You must be signed in to change notification settings - Fork 239
Fix check_app_name.sh: resolve FIXMEs and TODOs from #2118 #3126
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,72 +1,110 @@ | ||
| #!/bin/bash | ||
|
|
||
| set -e | ||
| shopt -s lastpipe # Run last command in a pipeline in the current shell. | ||
| shopt -s lastpipe | ||
|
|
||
| # Colors | ||
| LIGHTCYAN='\033[1;36m' | ||
| GREEN='\033[0;32m' | ||
| RED='\033[0;31m' | ||
| NC='\033[0m' # No Color | ||
| NC='\033[0m' | ||
|
Comment on lines
-10
to
+9
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason you removed these comments? They're kinda helpful in understanding the code better.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My mistake, I'll restore those comments. They do add clarity. |
||
|
|
||
| # Vars | ||
| SUCCESS=1 | ||
| CANONICAL_TITLE="Catima" | ||
|
|
||
| # peo is only temporarily allowed due to https://github.com/WeblateOrg/weblate/issues/17455 | ||
| ALLOWLIST=("ar" "bn" "fa" "fa-IR" "he-IL" "hi" "hi-IN" "kn" "kn-IN" "ml" "mr" "peo", "ta" "ta-IN" "zh-rTW" "zh-TW") # TODO: Link values and fastlane with different codes together | ||
| ALLOWLIST=("ar" "bn" "fa" "fa-IR" "he-IL" "hi" "hi-IN" "kn" "kn-IN" "ml" "mr" "peo" "ta" "ta-IN" "zh-rTW" "zh-TW") | ||
|
Comment on lines
15
to
+16
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given WeblateOrg/weblate#17455 is fixed this may be a good moment to remove peo :)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! I'll remove peo from the allowlist since the upstream issue is resolved. |
||
|
|
||
| declare -A FASTLANE_TITLES # lang -> title.txt app name | ||
| declare -A STRINGS_NAMES # lang -> strings.xml app_name | ||
|
|
||
| function get_lang() { | ||
| LANG_DIRNAME=$(dirname "$FILE" | xargs basename) | ||
| LANG=${LANG_DIRNAME#values-} # Fetch lang name | ||
| LANG=${LANG#values} # Handle "app/src/main/res/values" | ||
| LANG=${LANG:-en} # Default to en | ||
| LANG=${LANG_DIRNAME#values-} | ||
| LANG=${LANG#values} | ||
| LANG=${LANG:-en} | ||
|
Comment on lines
-20
to
+25
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason you removed these comments? They're kinda helpful in understanding the code better.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My mistake, I'll restore those comments. They do add clarity. |
||
| } | ||
|
|
||
| # FIXME: This function should use its own variables and return a success/fail status, instead of working on global variables | ||
| function check() { | ||
| # FIXME: This allows inconsistency between values and fastlane if the app name is not Catima | ||
| # When the app name is not Catima, it should still check if title.txt and strings.xml use the same app name (or start) | ||
| if echo "${ALLOWLIST[*]}" | grep -w -q "${LANG}" || [[ -z ${APP_NAME} ]]; then | ||
| return 0 | ||
| fi | ||
|
|
||
| if [[ ${FILE} == *"title.txt" ]]; then | ||
| if [[ ! ${APP_NAME} =~ ^${CANONICAL_TITLE} ]]; then | ||
| echo -e "${RED}Error: ${LIGHTCYAN}title in $FILE ($LANG) is ${RED}'$APP_NAME'${LIGHTCYAN}, expected to start with ${GREEN}'$CANONICAL_TITLE'. ${NC}" | ||
| SUCCESS=0 | ||
| fi | ||
| else | ||
| if [[ ${APP_NAME} != "${CANONICAL_TITLE}" ]]; then | ||
| echo -e "${RED}Error: ${LIGHTCYAN}app_name in $FILE ($LANG) is ${RED}'$APP_NAME'${LIGHTCYAN}, expected ${GREEN}'$CANONICAL_TITLE'. ${NC}" | ||
| SUCCESS=0 | ||
| function is_allowlisted() { | ||
| local lang="$1" | ||
| for entry in "${ALLOWLIST[@]}"; do | ||
| if [[ "$entry" == "$lang" ]]; then | ||
| return 0 | ||
| fi | ||
| fi | ||
| done | ||
| return 1 | ||
| } | ||
|
|
||
| # FIXME: This checks all title.txt and strings.xml files separately, but it needs to check if the title.txt and strings.xml match for a language as well | ||
| echo -e "${LIGHTCYAN}Checking title.txt's. ${NC}" | ||
|
|
||
| # --- Pass 1: collect title.txt values --- | ||
| echo -e "${LIGHTCYAN}Checking title.txt's.${NC}" | ||
| find fastlane/metadata/android/* -maxdepth 1 -type f -name "title.txt" | while read -r FILE; do | ||
| APP_NAME=$(head -n 1 "$FILE") | ||
|
|
||
| get_lang | ||
| check | ||
| done | ||
|
|
||
| echo -e "${LIGHTCYAN}Checking string.xml's. ${NC}" | ||
| FASTLANE_TITLES["$LANG"]="$APP_NAME" | ||
|
|
||
| if is_allowlisted "$LANG"; then | ||
| continue | ||
| fi | ||
|
|
||
| if [[ -n "$APP_NAME" && ! "$APP_NAME" =~ ^${CANONICAL_TITLE} ]]; then | ||
| echo -e "${RED}Error: ${LIGHTCYAN}title in $FILE ($LANG) is ${RED}'$APP_NAME'${LIGHTCYAN}, expected to start with ${GREEN}'$CANONICAL_TITLE'.${NC}" | ||
| SUCCESS=0 | ||
| fi | ||
| done | ||
|
|
||
| # --- Pass 2: collect strings.xml app_name values --- | ||
| echo -e "${LIGHTCYAN}Checking strings.xml's.${NC}" | ||
| find app/src/main/res/values* -maxdepth 1 -type f -name "strings.xml" | while read -r FILE; do | ||
| # FIXME: This only checks app_name, but there are more strings with Catima inside it | ||
| # It should check the original English text for all strings that contain Catima and ensure they use the correct app_name for consistency | ||
| APP_NAME=$(grep -oP '<string name="app_name">\K[^<]+' "$FILE" | head -n1) | ||
|
|
||
| get_lang | ||
| check | ||
|
|
||
| [[ -n "$APP_NAME" ]] && STRINGS_NAMES["$LANG"]="$APP_NAME" | ||
|
|
||
| if [[ -z "$APP_NAME" ]] || is_allowlisted "$LANG"; then | ||
| continue | ||
| fi | ||
|
|
||
| if [[ "$APP_NAME" != "$CANONICAL_TITLE" ]]; then | ||
| echo -e "${RED}Error: ${LIGHTCYAN}app_name in $FILE ($LANG) is ${RED}'$APP_NAME'${LIGHTCYAN}, expected ${GREEN}'$CANONICAL_TITLE'.${NC}" | ||
| SUCCESS=0 | ||
| fi | ||
|
|
||
| # Check all strings containing "Catima" use app_name for consistency | ||
| while IFS= read -r line; do | ||
| string_name=$(echo "$line" | grep -oP 'name="\K[^"]+') | ||
| string_val=$(echo "$line" | grep -oP '>[^<]+<' | tr -d '><') | ||
| if [[ "$string_name" != "app_name" && "$string_val" == *"Catima"* ]]; then | ||
| echo -e "${RED}Warning: ${LIGHTCYAN}string '$string_name' in $FILE ($LANG) hardcodes 'Catima' instead of using @string/app_name.${NC}" | ||
| fi | ||
| done < <(grep -E '<string name=".+">.*Catima.*</string>' "$FILE") | ||
| done | ||
|
|
||
| # --- Pass 3: cross-check title.txt vs strings.xml per language --- | ||
| echo -e "${LIGHTCYAN}Cross-checking title.txt vs strings.xml per language.${NC}" | ||
| for lang in "${!FASTLANE_TITLES[@]}"; do | ||
| fastlane_name="${FASTLANE_TITLES[$lang]}" | ||
| strings_name="${STRINGS_NAMES[$lang]:-}" | ||
|
|
||
| if [[ -z "$strings_name" ]]; then | ||
| continue | ||
| fi | ||
|
|
||
| if is_allowlisted "$lang"; then | ||
| continue | ||
| fi | ||
|
|
||
| # Strip subtitle after any dash variant or colon, then remove all whitespace | ||
| fastlane_appname=$(echo "$fastlane_name" | sed 's/[—–\-].*//' | sed 's/:.*//' | tr -d '[:space:]') | ||
| if [[ "$fastlane_appname" != "$strings_name" ]]; then | ||
| echo -e "${RED}Error: ${LIGHTCYAN}lang '$lang' title.txt app name='$fastlane_appname' does not match strings.xml app_name='$strings_name'.${NC}" | ||
| SUCCESS=0 | ||
| fi | ||
| done | ||
|
|
||
| if [[ $SUCCESS -eq 1 ]]; then | ||
| echo -e "\n${GREEN}Success!! All app_name values match the canonical title. ${NC}" | ||
| echo -e "\n${GREEN}Success!! All app_name values match the canonical title.${NC}" | ||
| else | ||
| echo -e "\n${RED}Unsuccessful!! Some app_name values did not match the canonical titles. ${NC}" | ||
| echo -e "\n${RED}Unsuccessful!! Some app_name values did not match the canonical titles.${NC}" | ||
| exit 1 | ||
| fi | ||
| fi | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like an improvement from a quick look. I can also confirm that Fastlane and values.xml get correctly compared. For example, if I manually change it:
Error: lang 'lt' title.txt app name='Catapp' does not match strings.xml app_name='Catima'.I think it still needs a bit of extra logic to deal with stuff like
esin values beinges-ESin Fastlane andzh-rCNin values beingzh-CNin Fastlane. I think we can probably automatically link most of those for more thorough checking.I'll look a bit deeper into this later but it seems a great help and it's great that it also detects all the places I wrongly hardcoded it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
greed, I'll add a mapping layer to normalize locale codes (e.g. es -> es-ES, zh-rCN -> zh-CN) before comparison so mismatches don't slip through.