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

[PT-BR] New sentences and some non-translated are now translated #1798

Open
wants to merge 6 commits into
base: untranslated-state
Choose a base branch
from

Conversation

jmceara
Copy link

@jmceara jmceara commented Dec 15, 2023

As requested by @tetele in #1768 (comment)

based on #1767

waiting for: home-assistant/core#105524

@jmceara
Copy link
Author

jmceara commented Dec 15, 2023

I hope it's ok now.

@llluis
Copy link

llluis commented Jan 10, 2024

There are currently 3 PRs for PT-BR with several overlapping intents:
#1798
#1783
#1413

I'm flagging them here so we can clear those upon merge.

PS: they are all missing the label lang:pt-br, don't know why the bot missed them.

Copy link

@llluis llluis left a comment

Choose a reason for hiding this comment

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

Great job @jmceara, thanks. I proposed some changes, let me know what you think.

sentences/pt-br/fan_HassTurnOff.yaml Show resolved Hide resolved
sentences/pt-br/weather_HassGetWeather.yaml Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

Should we add | abs to the if verification to cover for "-1 grau"?

Copy link
Author

Choose a reason for hiding this comment

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

Hi! I don't think it's necessary here. Can you elaborate a little more why you think we should use abs here? Tks!

@Jhonattan-Souza
Copy link

Hi guys, when will this PR be merged?
Who needs to approve

@tetele
Copy link
Contributor

tetele commented Jan 16, 2024

I can't find it atm, but I remember telling @jmceara somewhere that he shouldn't place all of the Brazilian Portuguese translations in this PR. If I haven't, I apologize and I'm saying it now.

So... the untranslated-state branch is strictly for exposing the untranslated weather and person states to the sentences, for better handling in some limited situations. That will get merged into main depending on what happens to the upstream PR.

But until then, if you want to have pt-br translated regardless of what happens in the core, you should create a PR with the translations against the main branch, not against the untranslated-state branch. If you do, @synesthesiam can take a look and decide to merge the PR and grant someone pt-br language leader status, so they can handle this stuff on their own.

Also, regarding this specific PR, I've approved the CI pipeline run and linting fails, so it can't be merged regardless. Run script/lint locally to fix the issues and push another commit.

@Jhonattan-Souza
Copy link

Jhonattan-Souza commented Jan 16, 2024

Hi @tetele , thank you !
@llluis @jmceara
Can we do this ?
If you guys agree, I can send this PR to the main branch

@jmceara
Copy link
Author

jmceara commented Jan 16, 2024

Hi @tetele , thank you !
@llluis @jmceara
Can we do this ?
If you guys agree, I can send this PR to the main branch

Agree!
I'm not sure why it failed on test. I have done all tests locally before send to github, all passed.

@v1k70rk4
Copy link
Member

v1k70rk4 commented Feb 6, 2024

@jmceara Hi, the issue here is that there are 16 space characters left in the 7th line of the sentences/pt-br/homeassistant_HassNevermind.yaml file.

If there are errors remaining after running the script/lint, it is advisable to reconfigure the environment using the script/setup command.

@v1k70rk4 v1k70rk4 requested review from luyzfernando08 and removed request for luyzfernando08 February 6, 2024 20:36
@gohlas
Copy link
Contributor

gohlas commented Feb 19, 2024

Hi @jmceara,
Could we try to fix and pull this?

@jmceara
Copy link
Author

jmceara commented Feb 19, 2024

Hi @jmceara, Could we try to fix and pull this?

Done! All tests passed, including lint.
Can you check now @Jhonattan-Souza @v1k70rk4 @llluis

@llluis
Copy link

llluis commented Feb 19, 2024

My vote doesn't count but I reviewed and approved anyways. :)

@v1k70rk4
Copy link
Member

If everything is perfect, then @luyzfernando08 it's your job to press the Approve and Run button :) You are the language leader.
Unfortunately, I don't understand Portuguese :) :D

@jmceara
Copy link
Author

jmceara commented Feb 19, 2024

If everything is perfect, then @luyzfernando08 it's your job to press the Approve and Run button :) You are the language leader. Unfortunately, I don't understand Portuguese :) :D

Well, I guess my application was not accepted LOL

@gohlas
Copy link
Contributor

gohlas commented Feb 20, 2024

Thanks @jmceara! That was a quick reply. :)
I am just going through more details here. Looks like this will not be merged as it is now.
According to @tetele, we need to make this changes against the main branch, not untranslated-state.
Regarding #1848 for weather and person, this PR already contains the changes for pt-br, should this be split in two different PRs? @tetele

@v1k70rk4
Copy link
Member

This branch into which the PR was merged is the "untranslated-state" branch, which as you also write, is intended for the handling of weather and person in "untranslated-state" to function if introduced.

What is currently in there will fit, it won't bother anyone :). But what you would like to use from this that is not related to person and weather, you will need to implement into the main as well.

It's important that any potentially missing parts of the _common.yaml file should definitely be added to the main branch too, to make the future merging easier.

@gohlas
Copy link
Contributor

gohlas commented Feb 20, 2024

Thanks @v1k70rk4 ,
Ok, then we can just create a new PR against main, without the changes in HassGetState.yaml and HassGetWeather.yaml.
Did I understand it right?

@v1k70rk4
Copy link
Member

@gohlas As you say :) If there is no conflict, there is nothing preventing this.

But you can also put the weather sentences in reduced mode into the main, so it will only display the temperature, but it will also work there.

For example, in the responses\pt-br\HassGetWeather.yaml file, this is the response:

language: pt-br
responses:
  intents:
    HassGetWeather:
      default: >
        {{ state.attributes.get('temperature') }} {{ state.attributes.get('temperature_unit') }}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants