-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
feat: Crowdin import automation #12698
Conversation
and affiliated utils, constants and types
track and save summary; add markdown checker step to getTranslations.ts
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
src/scripts/crowdin/import/README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to clean this up, and update this with the latest flow.
This README is an alternate approach to a huge comment block inside the script file, after that script was broken up.
Of note, as of the current branch commit, we still have the old scripts/crowdin-import.ts
script. This PR implements a new version of this as scripts/crowdin/import/
with main.ts
being the starting point.
We should decide what we want to do with the old scripts/crowdin-import.ts
script, options including:
A. Delete it in favor of the new version, which limits the manual functionality a bit
B. Add manual overrides into the new version
C. Keep both. Keep the original as-is, like a "v1", to be used in any event that we need to manually import buckets in the future; Meanwhile also keep the new one as a "v2", more tailored to the automated workflow (expects a list of buckets to be passed to it, and options are simplified)
Personally think option C could be nice, at least while we're in this transition... as to not alter the existing functioning script, while allowing us to build this new one to meet the new needs.
src/scripts/markdownChecker.ts
Outdated
} | ||
|
||
console.table(summary) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if the paths/line numbers are still clickable through this? It's a nice DX feature when resolving issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question... They would be if run locally, but this is not designed to be run by the user, so these would more be viewed in the GitHub Action console log... where no, they wouldn't be clickable.
Will keep this in mind... might be a matter of generating links somehow for each of these 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've semi-reverted the changes here... made a log
function that will log each event to the console with the previous levels (log
, warn
, or error
), and also tracks these in a list to be used for the PR body.
This should log them the same way as before (hence removal of the final console.table(summary)
, and thus should still be able to be run locally to get these clickable paths.
The PR summary will output a copy/paste-able code block with yarn markdown-checker
there as a convenience/reminder.
Fetches buckets with 100% approved phrases from Crowdin; in lieu of pulling from Notion board/API
Latest commits add in a script that fetches the approval status of each directory on Crowdin and generates the "buckets list". We need to test this, and likely debug some details, but otherwise the main thing left would be enabling the actually pushing of these branches to GitHub and opening PRs for each. The full "action" will not run until merged into dev. May be worth orchestrating this to only work for a couple languages, then bring it in and see how it does with those before removing that restriction. |
update get-translations.yml workflow
Extract for loop script into function for improved readability, uses "locale", uses --body-file, patches --title syntax
refactor: clean up postLangPRs, rm no-longer-used util functions, add decompress package types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks @wackerow!
I think this is basically ready to run as is, we can test it out at the start of May for all of the April reviews that are currently in progress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wackerow nice work, overall it looks good to me! I've left, mostly, refactor ideas that I'd like to see your opinions on.
=====
Been looking at the official GH/Crowdin action integration, it looks like it does most of functionality we need. Do you know what's stopping us from using it? couldn't we build our custom logic on top of it?
* @param repoLangCode Language code used within the repo | ||
* @returns void | ||
*/ | ||
export const scrapeDirectory = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function do more than just scrape the dir. We could break out the logic with something like this and simplify the function
function scrapeDirectory(dir, cb) {
// ...
ls.forEach(cb)
}
scrapeDirectory(dir, (path) => {
// do our logic
})
Having done this, we could move scrapeDirectory
to somewhere in @/libs/utils
since it is a reusable function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pettinarip Curious if you see this as blocking? This logic was ported forward from the previous crowdin-import.ts
file. Any chance we could handle this refactor separately? Similar with above comment about migrating things to @/lib/utils/crowdin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not a blocker, was more an refactor idea to make it clearer. Got it, makes sense what you are saying! we need to investigate a bit about those aliases and ways to use them in scripts.
* Some language codes used in the repo differ from those used by Crowdin. | ||
* This is used to convert any codes that may differ when performing folder lookup. | ||
*/ | ||
export const getCrowdinCode = (code: string): string => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we move some of these (if not all) functions to the @/lib/utils/crowdin
file?
I'd probably only keep the summary logic here since it is tightly related to this script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pettinarip If we do this we just need to make sure we don't import dependencies using @
syntax anywhere in the chain... In the src/scripts
folder we are typically running these with ts-node
which fails when it encounters that shorthand with code: 'MODULE_NOT_FOUND'
(at least with our current setup).
In src/lib
we use the @
import syntax, so if we want to mix them we may need to reverse that syntax in some cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! didn't know about that problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the pattern, I'd call this main.ts
since it is the main script file that is going to be executed.
Co-authored-by: Pablo Pettinari <pettinarip@gmail.com>
Co-Authored-By: Pablo Pettinari <pettinarip@gmail.com>
Interesting! I hadn't looked into that, but I've already got a handful of ideas for how we could iterate a v2 of this new approach. If you're okay with it I'd like to note this one for later. |
item.endsWith(".md") || | ||
item.endsWith(".svg") || | ||
item.endsWith(".xlsx") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
item.endsWith(".md") || | |
item.endsWith(".svg") || | |
item.endsWith(".xlsx") | |
item.endsWith(".md") |
I don't think we want to commit .svg and .xlsx file formats. There is a different process for importing images that @nloureiro and @lukassim have sorted out I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just have one comment above on the files being committed. Other than that, this seems good to go for me. I agree with @pettinarip about maybe checking out the integration, but I do think this can wait for later. This is working now and we should test this out before testing out that integration IMO.
Of course! just wanted to share that with you because it looked relevant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Co-authored-by: Corwin Smith <cssmittys@gmail.com>
Description
Adds a GitHub action cron job that runs a series of actions on the first of every month:
.crowdin/
crowdin-import.ts
worked in the past.Tasks
gh auth
and posting one PR per languageRelated Issue
Crowdin CI/CD efforts