Skip to content

Added action to prevent changing msgid #1351

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

Merged
merged 4 commits into from
Oct 11, 2023
Merged

Added action to prevent changing msgid #1351

merged 4 commits into from
Oct 11, 2023

Conversation

noamzaks
Copy link
Contributor

@noamzaks noamzaks commented Oct 11, 2023

See discussion in #1344.

This checks that all .po files changed in pull requests either contain changes in POT-Creation-Date (i.e. are run by the specific tool which should change msgid's), or don't change msgid's (by checking that each line changed is NOT between msgid and msgstr)

I have tested this on my fork, see valid changes where the action passes, and invalid changes where it doesn't.

The action also has output for failing/passing:
passing
failing

@mgeisler
Copy link
Collaborator

mgeisler commented Oct 11, 2023

Wow, this is great! Thank you for putting this together!

I can see what this does, but I wonder if it would be easier to understand if it was a Python script? They can be embedded directly into the GH action:

      - name: Update dependency graph
        run: |
          import fileinput, re, sys

          NAME = '${{ needs.setup.outputs.name }}'
          NEW_VERSION = '${{ needs.setup.outputs.new-version }}'

          for line in fileinput.input(inplace=True):
              sys.stdout.write(
                  re.sub(f'/images/{NAME}-.+\\.svg',
                         f'/images/{NAME}-{NEW_VERSION}.svg', line))
        shell: python3 {0} src/lib.rs

Though saying this, it would be better to just create a .py file inside the .github/workflows/ folder.

@mgeisler
Copy link
Collaborator

I think we can merge this as a shell script already, but if you find time, I would be interested in seeing it as a Python script instead. That would hopefully help with some of the more obscure (to me) lines such as

             if sed -n ${changed_line}p $filename | grep -q msgstr; then

@noamzaks
Copy link
Contributor Author

Definitely easier to understand (and write) as a Python script. Doesn't it "cost more minutes"? You need a setup-python action (I think). Didn't know there is Python built in to Actions workers.

I can turn this into a .py script inside .github/workflows, but then how will it run?

@mgeisler
Copy link
Collaborator

Definitely easier to understand (and write) as a Python script.

Awesome!

Doesn't it "cost more minutes"? You need a setup-python action (I think). Didn't know there is Python built in to Actions workers.

Yeah, it seems to be available right out of the box.

I can turn this into a .py script inside .github/workflows, but then how will it run?

You should be able to do

      - name: Configure Git user
        run: python3 .github/workflows/prevent-changing-msgid.py

(not tested... I think the path will be relative to the repository root, but perhaps it's relative to .github/workflows/?)

@noamzaks
Copy link
Contributor Author

Switched to Python and re-tested (links in the first post updated accordingly)

Copy link
Collaborator

@mgeisler mgeisler left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@mgeisler mgeisler merged commit b1cd19e into google:main Oct 11, 2023
mgeisler added a commit that referenced this pull request Oct 12, 2023
This enforces a consistent formatting for the PO files. The goal of
this is to avoid large diffs due to random and unnecessary
reformatting.

We use the format of `msgcat`: this is also waht `msgmerge` produces
and it’s easy to replicate for people by installing Gettext and
running `dprint fmt`.

This is a follow-up to #1351 which started enforcing that the `msgid`
fields don’t change due to reformatting in a PR.

If this turns out to be cumbersome, then we can disable it again.
mgeisler added a commit that referenced this pull request Oct 13, 2023
This enforces a consistent formatting for the PO files. The goal of
this is to avoid large diffs due to random and unnecessary
reformatting.

We use the format of `msgcat`: this is also waht `msgmerge` produces
and it’s easy to replicate for people by installing Gettext and
running `dprint fmt`.

This is a follow-up to #1351 which started enforcing that the `msgid`
fields don’t change due to reformatting in a PR.

If this turns out to be cumbersome, then we can disable it again.
mgeisler added a commit that referenced this pull request Oct 14, 2023
This enforces a consistent formatting for the PO files. The goal of
this is to avoid large diffs due to random and unnecessary
reformatting.

We use the format of `msgcat`: this is also waht `msgmerge` produces
and it’s easy to replicate for people by installing Gettext and
running `dprint fmt`.

The existing files were updated to match: 15 out of 18 translations
already followed the new format! I incremented the POT-Creation-Date
fields by one day: the precise date does not matter much since the
translations in question are incomplete and not yet published.

This is a follow-up to #1351 which started enforcing that the `msgid`
fields don’t change due to reformatting in a PR.

If this turns out to be cumbersome, then we can disable it again.
mgeisler added a commit that referenced this pull request Oct 14, 2023
This enforces a consistent formatting for the PO files. The goal of this
is to avoid large diffs due to random and unnecessary reformatting.

We use the format of `msgcat`: this is also waht `msgmerge` produces and
it’s easy to replicate for people by installing Gettext and running
`dprint fmt`.

This is a follow-up to #1351 which started enforcing that the `msgid`
fields don’t change due to reformatting in a PR.

If this turns out to be cumbersome, then we can disable it again.
@mgeisler
Copy link
Collaborator

Hi @noamzaks, I created #1364 to add a few comments to the script. You might want to add a few more to explain future maintainers what it does.

I also notes that we should

  • make the script read-only (remove the git reset)
  • make it use more Git plumbing commands instead of the high-level porcelain commands used today

It's not high-priority, but it would be nice to have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants