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

scripts/set-alias-page.py: fix status message for existing files #15389

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

Conversation

Zamoca42
Copy link
Collaborator

@Zamoca42 Zamoca42 commented Dec 30, 2024

Fixes: #14929

This PR addresses an issue in the set-alias-page.py script where it incorrectly shows “page would be added” for files that already exist and improves handling for diverse alias patterns.

Changes

  • Modified set_alias_page() function to check file existence before determining status.
  • Changed status determination logic from checking original_command to checking path.exists().
  • Replaced the regular expression logic in get_alias_command_in_page() with pre-parsed alias patterns specific to each language
    (e.g., "This command is an alias of" for English, "이 명령은" for Korean, "このコマンドは" for Japanese).

Before

  • Shows "page would be added" even for existing files that need to be updated.
  • Regular expression did not handle variations like GNU or trailing descriptions.

After

  • Shows "page would be added" only for new files.
  • Shows "page would be updated" for existing files that need to be updated.
  • Uses pre-parsed alias patterns for language-specific detection, improving robustness and readability.

@github-actions github-actions bot added documentation Issues/PRs modifying the documentation. tooling Helper tools, scripts and automated processes. translation Translate pages from one language to another. labels Dec 30, 2024
@tldr-bot
Copy link

Hello! I've noticed something unusual when checking this PR:

  • The page pages.ko/linux/i386.md is outdated, based on the command contents itself, compared to the English page.
  • The page pages.ko/linux/linux32.md is outdated, based on the command contents itself, compared to the English page.
  • The page pages.ko/linux/linux64.md is outdated, based on the command contents itself, compared to the English page.
  • The page pages.ko/linux/pacman-d.md is outdated, based on the command contents itself, compared to the English page.
  • The page pages.ko/linux/pacman-f.md is outdated, based on the command contents itself, compared to the English page.
  • The page pages.ko/linux/pacman-q.md is outdated, based on the command contents itself, compared to the English page.
  • The page pages.ko/linux/pacman-r.md is outdated, based on the command contents itself, compared to the English page.
  • The page pages.ko/linux/pacman-s.md is outdated, based on the command contents itself, compared to the English page.
  • The page pages.ko/linux/pacman-t.md is outdated, based on the command contents itself, compared to the English page.
  • The page pages.ko/linux/pacman-u.md is outdated, based on the command contents itself, compared to the English page.
  • The page pages.ko/linux/uname26.md is outdated, based on the command contents itself, compared to the English page.
  • The page pages.ko/linux/x86_64.md is outdated, based on the command contents itself, compared to the English page.

Is this intended? If so, just ignore this comment. Otherwise, please double-check the commits.

@tldr-bot
Copy link

The build for this PR failed with the following error(s):

would reformat /home/runner/work/tldr/tldr/scripts/set-alias-page.py

Oh no! 💥 💔 💥
1 file would be reformatted, 6 files would be left unchanged.

Please fix the error(s) and push again.

@sebastiaanspeck
Copy link
Member

Could you remove the Korean changes? I think you have committed them by accident when testing

@sebastiaanspeck sebastiaanspeck changed the title scripts/set-alias-page.py: Fix status message in set-alias-page.py for existing files scripts/set-alias-page.py: Fix status message for existing files Dec 31, 2024
Copy link
Member

@sebastiaanspeck sebastiaanspeck left a comment

Choose a reason for hiding this comment

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

After removing the Korean changes (or moving them to a separate PR), this LGTM!

@Zamoca42
Copy link
Collaborator Author

Could you remove the Korean changes? I think you have committed them by accident when testing

Let's separate the Korean alias page into a different PR

@Zamoca42 Zamoca42 removed the translation Translate pages from one language to another. label Dec 31, 2024
@sebastiaanspeck sebastiaanspeck changed the title scripts/set-alias-page.py: Fix status message for existing files scripts/set-alias-page.py: fix status message for existing files Dec 31, 2024
@sebastiaanspeck
Copy link
Member

A minor fix needs to be made I think.

When running it (scripts/set-alias-pages.py -l nl -S), it also changed a lot of pages.nl/osx/g....md files, updating false-positives:

image

root / "pages" / command,
r"^> This command is an alias of `(.+)`\.$",
r"^> This command is an alias of(?: \w+)? `([^`]+)`(?:,.*)?",
Copy link
Member

@sebastiaanspeck sebastiaanspeck Dec 31, 2024

Choose a reason for hiding this comment

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

This regex is not completely working correctly anymore:

CHECKING tldr-pages/tldr/pages/osx/gsum.md using the regex: ^> This command is an alias of(?: \w+)? `([^`]+)`(?:,.*)?...
RESULT tldr-pages/tldr/pages/osx/gsum.md using the regex: ^> This command is an alias of(?: \w+)? `([^`]+)`(?:,.*)?: COMMAND_NAME= sum

I expected it to be -p linux sum.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems that the command in the description and the tldr command might differ. I’ll try modifying the get_alias_command_in_page() function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tested the changes by creating and running an arbitrary test function, and the updated logic works as expected for most files.

❯ python scripts/set-alias-page.py --test pages/osx/gsum.md
Testing file: pages/osx/gsum.md
Content: # gsum

> This command is an alias of GNU `sum`.

- View documentation for the original command:

`tldr -p linux sum`

Original command: sum
Documentation command: -p linux sum

❯ python scripts/set-alias-page.py --test pages.nl/osx/gsum.md
Testing file: pages.nl/osx/gsum.md
Content: # gsum

> Dit commando is een alias van `-p linux sum`.

- Bekijk de documentatie van het originele commando:

`tldr -p linux sum`

Original command: -p linux sum
Documentation command: -p linux sum

❯ python scripts/set-alias-page.py --test pages/osx/g\[.md
Testing file: pages/osx/g[.md
Content: # g[

> This command is an alias of GNU `[`.

- View documentation for the original command:

`tldr -p linux [`

Original command: [
Documentation command: -p linux [

❯ python scripts/set-alias-page.py --test pages.nl/osx/g\[.md
Testing file: pages.nl/osx/g[.md
Content: # g[

> Dit commando is een alias van `-p linux [`.

- Bekijk de documentatie van het originele commando:

`tldr -p linux [`

Original command: -p linux [
Documentation command: -p linux [

Copy link
Member

Choose a reason for hiding this comment

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

When testing with the latest version, it is still "broken". --test does not exist anymore. I use python scripts/set-alias-page.py -Snl nl which still results in the long list with the osx/g*.md.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was just for output, but now added a function for testing.

Copy link
Member

Choose a reason for hiding this comment

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

The function to test isn't needed. It was just not working as expected.

@@ -241,9 +317,23 @@ def main():
parser = create_argument_parser(
"Sets the alias page for all translations of a page"
)
parser.add_argument("command", type=str, nargs="?", default="")
parser.add_argument(
"original_command",
Copy link
Member

Choose a reason for hiding this comment

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

Please update the usage at the beginning of the file to rename "command" to "original command" and add "documentation command".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added documentation examples for 'ORIGINAL_CMD' and 'DOC_CMD.' While testing, I found that commands like python3 scripts/set-alias-page.py -P osx/gsum sum '-p linux sum' failed due to the -p argument being incorrect. To fix this, I changed the page argument from -p to -P.

sebastiaanspeck and others added 3 commits January 2, 2025 17:08
- Extract locale alias pattern logic into a separate function for better readability
- Change -p to -P to avoid collision with tldr's -p flag in documentation command
  (e.g., "-p linux sum" in "python3 scripts/set-alias-page.py -P osx/gsum sum '-p linux sum'")
- Update documentation to use consistent command argument names (ORIGINAL_CMD, DOC_CMD)
- Change bullet points to asterisks in examples for better visibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues/PRs modifying the documentation. tooling Helper tools, scripts and automated processes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update on Missing Alias Pages in Translation Dashboard for ko
3 participants