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

improvement: better 'strings' message to maintainers #17160

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

david-allison
Copy link
Member

@david-allison david-allison commented Sep 29, 2024

We had a few cases where the process was not followed correctly

Improving the message should reduce the chance of this happening again

Fixes

Approach

  • Update the message

How Has This Been Tested?

Test results (on an issue, rather than a PR):

https://redirect.github.com/david-allison/Anki-Android/issues/42#issuecomment-2381427055

Learning (optional, can help others)

GitHub Actions are hard to test

"PR" URL was changed in Issue 15887:

- https://github.com/ankidroid/Anki-Android/pulls/app%2Fgithub-actions
+ https://github.com/ankidroid/Anki-Android/pulls/mikehardy-machineaccount

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Copy link
Member

@Arthur-Milchior Arthur-Milchior left a comment

Choose a reason for hiding this comment

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

As someone who made the mistake, I fear, twice, I am, to be frank, not entirely sure it'll help.
If other people are like me, the issue is not only that the message is unclear.
The issue could be that it appears after the creation of the PR, and there may be a lot of discussion between the automated message and the time it's worth merging the PR.

Maybe having this appear after an approval would help. But I doubt it. Because, most importantly, that seems to be noise. Same as the "This is your first PR, welcome to ankidroid" and, worst offender "this is your monthly reminder you can use open collective".
I'm really not used to see messages that are actually relevant here.

On the other paw, now that I know that string must be synced, I don't expect I'll do the message again.

.github/workflows/label.yml Outdated Show resolved Hide resolved
> **Maintainers**: This PR contains https://github.com/ankidroid/Anki-Android/labels/Strings changes

1. [Sync Translations](https://github.com/ankidroid/Anki-Android/actions/workflows/sync_translations.yml) before merging this PR and wait for the action to complete
2. Find the [auto-generated PR](https://github.com/ankidroid/Anki-Android/pulls/app%2Fgithub-actions) and open and close the PR to perform a lint check. Review and merge the PR to sync all user-submitted translations
Copy link
Member

Choose a reason for hiding this comment

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

If you want to be very specific in the message, I think a point that is simply about the review should be added.
It was very unclear to me what kind of review I could do given that I don't speak most languages.

  1. Review that translations seems legit. Most troll can be easily detected (not the same character set as the remaining of the field, insults written in English...).
    3'. If there are errors do [what should be done. Or put a link to there]
  2. Merge the string sync PR.

....

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushback: "How to do a code review" is out of scope here.

This should be a 'what' to do, rather than a 'how' to do it

@mikehardy
Copy link
Member

@Arthur-Milchior only replying to note I see this:

worst offender "this is your monthly reminder you can use open collective".

...happy to do it some other way but I can't think of a way to reliably inform people at the moment?

@Arthur-Milchior
Copy link
Member

@mikehardy No, I've no better idea.
Ideally, I'd just block this message, so that it stops popping in my notification for every PR in the ankidroid repo. But as far as I can see, I can't just block one user/bot. (Unless I block them for the whole repo, which I obviously won't do)

My point is only that, once a month (on average) there is a bunch of notification that I can just ignore. I just select them and mark the notification as "read".

I can't imagine any way to improve things here. At least not at AnkiDroid level, it would require a change in Github behaviour.

To take a step back, my main point was not about any specific message. Just about the fact that, I kind of learned that the bot's message are very repetitive, and so it's hard to pay attention to them.
But that's great. It means that any newcomer could read it once or twice. And any oldcomer could just state "great, this is time to ask to be paid!"

@david-allison david-allison changed the title improvement: improve 'strings' message to maintainers improvement: 'strings' message to maintainers Sep 30, 2024
@david-allison david-allison changed the title improvement: 'strings' message to maintainers improvement: better 'strings' message to maintainers Sep 30, 2024
@david-allison
Copy link
Member Author

david-allison commented Sep 30, 2024

As someone who made the mistake, I fear, twice, I am, to be frank, not entirely sure it'll help.
If other people are like me, the issue is not only that the message is unclear.
The issue could be that it appears after the creation of the PR, and there may be a lot of discussion between the automated message and the time it's worth merging the PR.

I'm hoping that using eye-catching admonitions reduces the chance of bad merges from happening.

There's a number of further automation steps which we can discuss & take which could block PR merges with strings, but I feel that this effort would be better spent on streamlining the translation sync process.

Specifically:

@mikehardy
Copy link
Member

Remove the need to open + close PRs

That one in particular is galling to me. It has to be something about the workflow syntax and what events trigger it. I can't believe a push on the branch doesn't do it ? maybe it's a different event? I haven't investigated but what a silly thing to have to do, and personally frustrating because...generally I'm good with workflows but that one resisted my previous attempts

@david-allison
Copy link
Member Author

david-allison commented Sep 30, 2024

GitHub has restrictions on recursive workflow runs, and recommends a machine account to resolve it

Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

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

I was already ok with the changes from the issue's discussion.

@lukstbit lukstbit added the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Sep 30, 2024
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Sep 30, 2024
@david-allison
Copy link
Member Author

david-allison commented Sep 30, 2024

No need to open & close the PR. Fixed today

@david-allison david-allison removed the Needs Author Reply Waiting for a reply from the original author label Sep 30, 2024
@david-allison
Copy link
Member Author

Updated to:


Important

Maintainers: This PR contains Strings changes

  1. Sync Translations before merging this PR and wait for the action to complete
  2. Review and merge the auto-generated PR in order to sync all user-submitted translations
  3. Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Sep 30, 2024
@david-allison
Copy link
Member Author

Updated the URL to:

https://github.com/ankidroid/Anki-Android/pulls/mikehardy-machineaccount

We had a few times where the process was not followed correctly

Improving the message should reduce the chance of this happening again

Additional links further streamline the process for
those who do not need the knowledge transfer

Test results:

https://redirect.github.com/david-allison/Anki-Android/issues/42#issuecomment-2381427055

Fixes 16980

Note: "PR" URL was changed in Issue 15887

```diff
- https://github.com/ankidroid/Anki-Android/pulls/app%2Fgithub-actions
+ https://github.com/ankidroid/Anki-Android/pulls/mikehardy-machineaccount
```
@david-allison david-allison removed the Needs Author Reply Waiting for a reply from the original author label Sep 30, 2024
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

tidy set of quality-of-life improvements in our automation, considering all the related changes together. Very nice

@mikehardy mikehardy added this pull request to the merge queue Sep 30, 2024
Merged via the queue into ankidroid:main with commit 715fa92 Sep 30, 2024
9 checks passed
@github-actions github-actions bot added this to the 2.19 release milestone Sep 30, 2024
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Sep 30, 2024
@david-allison david-allison deleted the 16980 branch September 30, 2024 21:13
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.

Proposal: improve the 'Translation Sync' message
4 participants