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 translation for Brazilian Portuguese #2535

Merged
merged 17 commits into from Oct 11, 2021
Merged

Added translation for Brazilian Portuguese #2535

merged 17 commits into from Oct 11, 2021

Conversation

piantino
Copy link
Contributor

@piantino piantino commented Aug 6, 2021

Hi,

This commit is enough?

Best regards,

@ultrabug
Copy link
Member

Hello @piantino , no it's not enough but you're on the right path

Please check #2525 for a nice and complete PR reference

@prcr
Copy link

prcr commented Sep 1, 2021

Should this translation use the language code pt_BR instead, in case another contributor wishes to add the European Portuguese translation at a later time?

@piantino
Copy link
Contributor Author

piantino commented Sep 2, 2021

I agree @prcr , we will change for pt_BR.
Thank you.

@piantino
Copy link
Contributor Author

piantino commented Sep 7, 2021

Hi @ultrabug and @prcr .

Update done!
Let us know if anything more is necessary!

@ultrabug
Copy link
Member

ultrabug commented Sep 7, 2021

Thanks mates, please check #2564

There is no pt_BR locale support from lunr.js so this will not get in as-is I'm afraid

@prcr
Copy link

prcr commented Sep 8, 2021

There is no pt_BR locale support from lunr.js so this will not get in as-is I'm afraid

OK, so perhaps it's simpler at this time to just have a pt translation.

@piantino, you could even submit this as the Portuguese translation instead of specifying as Brazilian Portuguese, since the only word that would probably be different in European Portuguese is "Buscar" (it's more common to translate "Search" to "Pesquisar"). 👍

@ultrabug
Copy link
Member

ultrabug commented Sep 8, 2021

please have a look at #2565 too, pt_BR might be added without breaking mkdocs anymore if both MR are accepted but the search engine will only work for pt, not pt_BR so I guess it's up to you

@prcr
Copy link

prcr commented Sep 8, 2021

Thanks @ultrabug, I think that #2565 makes a lot of sense and will make supporting languages that support the locale territory much more convenient. 🚀

So, I guess it's up to @piantino and @Nibuitoni if they wish to submit this translation as Brazilian Portuguese or just Portuguese. 🇧🇷 🇵🇹 Sorry if I made things more confusing!

@oprypin
Copy link
Member

oprypin commented Sep 8, 2021

Hmm sorry I haven't followed this in any detail at all. Also I suppose this isn't the exact issue to comment this on, because it's a more general decision.

But to me it appears very clear that ideally we'd be able to specify pt_BR while also being able to give pt to the search plugin if that's what it wants.

@ultrabug
Copy link
Member

ultrabug commented Sep 8, 2021

But to me it appears very clear that ideally we'd be able to specify pt_BR while also being able to give pt to the search plugin if that's what it wants.

What about zh_CN then? while pt is supported by lunr, zh is not for instance

@oprypin
Copy link
Member

oprypin commented Sep 8, 2021

@ultrabug Then it seems like a manual mapping is needed. Or wait, does lunr know on Python side which languages it supports? In that case try-except with decreasing verbosity might be possible to apply.

@ultrabug
Copy link
Member

ultrabug commented Sep 9, 2021

Or wait, does lunr know on Python side which languages it supports?

We have it all in our own source code here:

mkdocs/contrib/search/lunr-language
├── lunr.ar.js
├── lunr.da.js
├── lunr.de.js
├── lunr.du.js
├── lunr.es.js
├── lunr.fi.js
├── lunr.fr.js
├── lunr.hu.js
├── lunr.it.js
├── lunr.ja.js
├── lunr.jp.js
├── lunr.multi.js
├── lunr.nl.js
├── lunr.no.js
├── lunr.pt.js
├── lunr.ro.js
├── lunr.ru.js
├── lunr.stemmer.support.js
├── lunr.sv.js
├── lunr.th.js
├── lunr.tr.js
├── lunr.vi.js
└── tinyseg.js

In that case try-except with decreasing verbosity might be possible to apply.

Like :

  • try the language + territory first (pt_BR) -> if present, no log
  • else try the root language (pt) -> if present, INFO
  • else default to english -> WARN

That's what you mean?

@piantino
Copy link
Contributor Author

Thanks @ultrabug, I think that #2565 makes a lot of sense and will make supporting languages that support the locale territory much more convenient. rocket

So, I guess it's up to @piantino and @Nibuitoni if they wish to submit this translation as Brazilian Portuguese or just Portuguese. brazil Portugal Sorry if I made things more confusing!

Hi everyone,

We would keep this Pull Request in pt_BR, so I updated this to no conflict with the last master commit.
So, we will wait for #2564 and #2565.

Thank's.

@ultrabug
Copy link
Member

ultrabug commented Oct 8, 2021

I'm proposing #2602 to address the comments I had

@@ -103,6 +103,7 @@ supports the following options:
* `fr`: French
* `es`: Spanish
* `ja`: Japanese
* `pt-BR`: Portuguese (Brazil)
Copy link
Member

Choose a reason for hiding this comment

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

I think these - should be underscores (_), right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ops, sorry it was my fault. I made the adjust.

@oprypin
Copy link
Member

oprypin commented Oct 10, 2021

Could you merge master branch again?

@ultrabug Seems like we should exclude foreign language files from English spelling checks (codespell) 😄

But the PR authors can ignore that check

@piantino
Copy link
Contributor Author

Could you merge master branch again?

@ultrabug Seems like we should exclude foreign language files from English spelling checks (codespell) smile

But the PR authors can ignore that check

@oprypin merge done! =)

@oprypin
Copy link
Member

oprypin commented Oct 11, 2021

Oops, sorry about another merge conflict 😵
As a note for how to resolve it: the order of locales in the doc should be kept alphabetic.

@oprypin
Copy link
Member

oprypin commented Oct 11, 2021

@ultrabug Do you think this one is good to go? I couldn't find any problem with this.

@piantino
Copy link
Contributor Author

piantino commented Oct 11, 2021

Oops, sorry about another merge conflict dizzy_face As a note for how to resolve it: the order of locales in the doc should be kept alphabetic.

You are right, keep alphabetic order avoid this kind of merge problem.
A fixed it and squash the commits.

@oprypin
Copy link
Member

oprypin commented Oct 11, 2021

I strongly prefer that the squashing didn't happen. That drops all review history (both in terms of information and for keeping track of what's already reviewed) and has 0 advantages. And weren't there multiple authors in these commits?
If you'd be up for it, could you please undo the squash and instead push another merge commit. Starting with:

$ git reset --hard 20f202d1b73e69e8987433508f93aba6d116641e   # last non-forced commit

@piantino
Copy link
Contributor Author

I strongly prefer that the squashing didn't happen. That drops all review history (both in terms of information and for keeping track of what's already reviewed) and has 0 advantages. And weren't there multiple authors in these commits? If you'd be up for it, could you please undo the squash and instead push another merge commit. Starting with:

$ git reset --hard 20f202d1b73e69e8987433508f93aba6d116641e   # last non-forced commit

I tried to organize the commit messages but I forgot the impact of squash, sorry for that.
Merge done.

Copy link
Member

@oprypin oprypin left a comment

Choose a reason for hiding this comment

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

Thank you

@ultrabug
Copy link
Member

Obrigado (works for both pt and BR) ;)

@ultrabug
Copy link
Member

I was about to merge, but now I'm confused about #2615 ?

@oprypin
Copy link
Member

oprypin commented Oct 11, 2021

@ultrabug Ah that's clearly some accident after the complicated branch manipulation. We can ignore that

@ultrabug ultrabug merged commit 815af48 into mkdocs:master Oct 11, 2021
@piantino
Copy link
Contributor Author

@ultrabug Ah that's clearly some accident after the complicated branch manipulation. We can ignore that

@ultrabug I made some mistakes, I thought that was in my repository...

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.

None yet

5 participants