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

github: Fix weblate action's rights, and update generated code. #1192

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

Conversation

alexmv
Copy link
Contributor

@alexmv alexmv commented Dec 20, 2024

No description provided.

@chrisbobbe chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Dec 20, 2024
@chrisbobbe chrisbobbe requested a review from gnprice December 20, 2024 16:25
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! One substantive comment, one interesting nit, and one boring one.

Comment on lines +28 to +30
git clone --depth=1000 -b main https://github.com/flutter/flutter ~/flutter
TZ=UTC git --git-dir ~/flutter/.git log -1 --format='%h | %ci | %s' --date=iso8601-local
echo ~/flutter/bin >> "$GITHUB_PATH"
Copy link
Member

Choose a reason for hiding this comment

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

This will need an update to match recent changes in ci.yml — see #1177 and the two PRs that closed it. (Just one command to add, thankfully.)

Probably this is a sign we should factor this part out as a tiny shell script in its own file. I'd be happy to do that as a followup after this PR, though.


- name: Update generated code
run: |
./tools/check l10n --fix
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
./tools/check l10n --fix
tools/check l10n --fix

The rule is that once there's a slash in the command name, it means a file by that name, and there's no search in PATH (or for shell functions or builtins). Docs:
https://www.gnu.org/software/bash/manual/bash.html#Command-Search-and-Execution
(and POSIX says the same thing but is more annoying to reference:
https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/utilities/V3_chap02.html#tag_18_09_01 )

So a leading ./ is useful only as a trick to get a slash in there when the file's name would otherwise have no slash.

Comment on lines 13 to 15
- uses: actions/checkout@v4

- name: Fetch and merge from Weblate
Copy link
Member

Choose a reason for hiding this comment

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

nit: once these steps are separated by blank lines, I feel like I want the enclosing jobs: to be separated by a blank line from its siblings — otherwise it feels like a bit of a visual-hierarchy inversion

@gnprice
Copy link
Member

gnprice commented Dec 20, 2024

Also it'd be great to get to test this (and similar PRs) before merging it to main.

I floated a possible solution for that just now in the chat thread:
https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/weblate.20translations.20.28.23F276.29/near/2011576

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants