Skip to content

/ Localisation

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

Jump to bottom

Improve translation check action #1998

Open
marcinkunert opened this issue Oct 21, 2020 · 2 comments
Open

Improve translation check action #1998

marcinkunert opened this issue Oct 21, 2020 · 2 comments

Comments

@marcinkunert
Copy link
Contributor

@marcinkunert marcinkunert commented Oct 21, 2020 •
edited

Few things came up after seeing the script in action. Short intro how it works:

  1. Checkout the branch of the fork (newest commit) to the pr directory
  2. Checkout the master branch of the fork (not OpenRCT/Localisation)
  3. Calculate and checkout the merge-base of the two as master

Using the image as example, pr will point to ca1ff5e and master will point to 0339427

image

Problems with that:

  • If the pull request is created using the fork master branch it will not detect any changes (as the merge-base is the same as the newest commit). Example here: #1996. Already mentioned here: #1992 (comment)
  • If the fork master branch is not rebased it will not contain the newest or not contain at all the check script. Example here: #1997. This should be easy to fix: additionally checkout the master of OpenRCT2/Localisation and run the script from there
  • Do not post a comment if there are no changes
@marcinkunert marcinkunert changed the title Improve translation check action [editing...] Improve translation check action Oct 21, 2020
@marcinkunert marcinkunert changed the title [editing...] Improve translation check action Improve translation check action Oct 21, 2020
@tupaschoal
Copy link
Member

@tupaschoal tupaschoal commented Oct 21, 2020

This looks like a great enhancement and I think it touches on some limitations that @Gymnasiast had raised before.
@marcinkunert marcinkunert mentioned this issue Oct 24, 2020
Merged
@marcinkunert
Copy link
Contributor Author

@marcinkunert marcinkunert commented Oct 25, 2020

PR from master seems to work now (example here #2010). Only thing to do is to not post a comment if there are no changes or the changes do not affect the translation files at all.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
@marcinkunert @tupaschoal
You can’t perform that action at this time.