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

Added Russian language #363

Merged
merged 7 commits into from
Jan 19, 2021
Merged

Added Russian language #363

merged 7 commits into from
Jan 19, 2021

Conversation

estim
Copy link
Contributor

@estim estim commented Oct 1, 2020

Translation for Russian language

Translation for Russian language
@aplaice
Copy link
Collaborator

aplaice commented Oct 2, 2020

Wow! Thanks for this!

Other than CSV quoting normalisation (which can be fixed with a composer index and which is irrelevant given the impending switch to brain-brew and on a semi-cursory look, this looks great! I don't speak Russian, though, and can only just-about parse Cyrillic, so all I can say that the names look vaguely correct and the structure of the additions appears right.

However, I think that the consensus is that we'll be waiting until we switch to brain-brew to add any more languages*, so merging this will probably need to wait until then. (None of your work will be wasted, since the translation will be easily transferable to the new system.)

* otherwise the CSV will become completely unwieldy

@estim
Copy link
Contributor Author

estim commented Oct 2, 2020

Cool!
Do I need to fix anything with composer index? Or you'll wait for the new system and everything will be parsed automatically?

@aplaice
Copy link
Collaborator

aplaice commented Oct 2, 2020

Do I need to fix anything with composer index? Or you'll wait for the new system and everything will be parsed automatically?

No need, everything will be automatically parsed.

(The issue is that anki-dm, our current system, uses a weird, non-standard CSV quoting convention, "inherited" from PHP, while you used the most common (normal) CSV field quoting convention. The new system will be able to cope just as well with either.)

@ohare93
Copy link
Member

ohare93 commented Dec 21, 2020

I'll make an update for this PR after #364 is pulled 👍

@ohare93
Copy link
Member

ohare93 commented Jan 17, 2021

PR created 👍 @estim just has to pull that, and this one will only differ in the Russian changes 👍 (plus I removed the sorting in the recipes, as it was annoying)

See here to preview all the changes.

@axelboc
Copy link
Collaborator

axelboc commented Jan 18, 2021

@ohare93 I've given you contributor access to the repo, so you may be able to force-push to this branch, if it helps.

@ohare93
Copy link
Member

ohare93 commented Jan 18, 2021

@ohare93 I've given you contributor access to the repo, so you may be able to force-push to this branch, if it helps.

Perhaps there are some Git tricks I am unaware of, but wouldn't I need to push to estim:master, the origin of this PR? 🤔

Thanks for the Contributor status though! 😁 I am happy to fix it, it is is indeed in my power.

@axelboc
Copy link
Collaborator

axelboc commented Jan 18, 2021

Yes, GitHub allows exactly that. It comes up as an option when you open a PR from a fork, and it's enabled by default, so unless @estim disabled it, a force-push to estim:master should work. 🤞

(This is a purely "formal" merge — it contains @ohare93's import of
the Russian translation without changes.)
@aplaice
Copy link
Collaborator

aplaice commented Jan 18, 2021

Perhaps to avoid force-pushing (which "feels" ugly and will make it harder to keep track of @estim's original translation, in the unlikely case we want to cross-reference something (not that I'm doubting BrainBrew's perfect accuracy! :))) the last commit could be made a "formal" merge?

Also: Hooray for your becoming an official contributor of the repository! :)

@aplaice
Copy link
Collaborator

aplaice commented Jan 18, 2021

Yes, GitHub allows exactly that. It comes up as an option when you open a PR from a fork, and it's enabled by default, so unless @estim disabled it, a force-push to estim:master should work. crossed_fingers

I think that @estim hasn't disabled it, since GitHub tells me that I can "Add more commits by pushing to the master branch on estim/anki-ultimate-geography." However, I think that that can only be done via CLI git rather than via the web interface.

@ohare93
Copy link
Member

ohare93 commented Jan 18, 2021

I think that @estim hasn't disabled it, since GitHub tells me that I can "Add more commits by pushing to the master branch on estim/anki-ultimate-geography."

Adding commits to Estim's branch is normal tho 🤔 not "pushing to a PR". I made a PR immediately for this purpose, into Estim's master, as you commented and pushed to.

However, I think that that can only be done via CLI git rather than via the web interface.

Sorry, I'm not seeing it! 😅 gh pr checkout 363 results in a 'not possible to fast-forward' error.

I am happy to wait a few days for Estim to react and pull 😄 after which the last touches about the translation can be confirmed and checked in 👍

@estim
Copy link
Contributor Author

estim commented Jan 18, 2021

I am happy to wait a few days for Estim to react and pull 😄 after which the last touches about the translation can be confirmed and checked in 👍

I'm not super familiar with Github and can't be sure that I'm doing something right / wrong. I've just seen your pull request in my master and merged it. What do I need to do now? - Make a pull request here again?

@ohare93
Copy link
Member

ohare93 commented Jan 18, 2021

@estim I hope you're still online, I literally updated the PR with the latest from Aplaice 4 mins after you merged 😆 I opened a new PR, please pull that one, then all is well!

UPDATED: Russian translation in brain brew
@ohare93
Copy link
Member

ohare93 commented Jan 18, 2021

Done! We are good to merge 😁 I don't believe any of us speak Russian, so if perhaps you could have a look over the Csvs and make sure all looks good (that nothing went wrong during our changes) that would be much appreciated 👏

@aplaice @axelboc anything left to do apart from that? 🙂

@aplaice
Copy link
Collaborator

aplaice commented Jan 18, 2021

Thanks for your amazing work @ohare93 and @estim!

Ideally, the list of languages in README.md and src/headers/desc.html would be updated, but that can be done later, in one go (once we have even more languages!).

Looking at the CSV changes (with git diff --word-diff-regex="[^[:space:],]+") I think that nothing has gone wrong, but it might indeed be useful if you (@estim) briefly looked over the CSVs (in src/data/; the last column is the Russian translation), if/when you have time (but no pressure!).

@estim
Copy link
Contributor Author

estim commented Jan 18, 2021

Checked and it seems that everything is still ok!

@ohare93
Copy link
Member

ohare93 commented Jan 19, 2021

Thanks for your amazing work @ohare93 and @estim!

🚀

Ideally, the list of languages in README.md and src/headers/desc.html would be updated, but that can be done later, in one go (once we have even more languages!).

Oh yes, good idea to do it all at the end 👍 If that's the case, we're ready to go! 👏

As @axelboc seemed raring to go on this one, I will merge it! 😁

@ohare93 ohare93 merged commit 1cc8d4d into anki-geo:master Jan 19, 2021
@axelboc
Copy link
Collaborator

axelboc commented Jan 19, 2021

Thanks @ohare93, and of course thank you @estim! I've had a quick look this morning and nothing jumps out at me 👍

@axelboc axelboc added the translation Translating the deck to a new language label Jan 20, 2021
@axelboc axelboc added this to the v4.1 milestone Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
translation Translating the deck to a new language
Development

Successfully merging this pull request may close these issues.

None yet

4 participants