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

[PL] - big update for current intents and added new intents for cover, valve, vacuum, media_player, person, todo, weather and more #1996

Merged
merged 69 commits into from
Mar 14, 2024

Conversation

witold-gren
Copy link
Contributor

No description provided.

@Uriziel01
Copy link
Contributor

@witold-gren I did a small lookaround and I do have some small remarks, give me few hours to write them down for you after 4PM. Sorry for the delay, I have a 3 week baby, hard to find any free time 🙂

@deejay1
Copy link
Contributor

deejay1 commented Mar 5, 2024

@Uriziel01 congrats and condolences at the same time, you won't have any free time for the next 10 years at least (speaking as a father of two ;)

@witold-gren
Copy link
Contributor Author

Guys.. come on.. I have four children.. 😉 And as you can see, I'm not short on time..

Back to the business. I updated the person entities to make them more flexible when asking questions (I followed PR as example #1779). Today is the last day of my updates. I'm leaving tomorrow, so unfortunately I won't be able to update this PR. I won't be available until Monday.

@witold-gren
Copy link
Contributor Author

Hey @Uriziel01 @synesthesiam,
since there are still no comments or urgent requests for any changes, please include this PR in the master version, as I would like it to be available in today's release. We can make some changes in the next PR if they will be required. I will also be unlocked to work on sensors and binary_sensors sentences.

@Uriziel01
Copy link
Contributor

Hey @Uriziel01 @synesthesiam, since there are still no comments or urgent requests for any changes, please include this PR in the master version, as I would like it to be available in today's release. We can make some changes in the next PR if they will be required. I will also be unlocked to work on sensors and binary_sensors sentences.

I think I for once agree with that sentence, I'm going through it aws but in reality 80 files at the same time changed and refactered
within 61 commits makes it almost unreviewable properly. If you merge it before I'm finished that's fine, if I'm first and I find anything in this enormous pile that's also good. Win-win 🙂

@witold-gren
Copy link
Contributor Author

Sorry, but unfortunately I don't have permission to merge any PR related with polish language.

tests/pl/homeassistant_HassGetState.yaml Show resolved Hide resolved
sentences/pl/_common.yaml Outdated Show resolved Hide resolved
sentences/pl/_common.yaml Outdated Show resolved Hide resolved
sentences/pl/cover_HassGetState.yaml Show resolved Hide resolved
sentences/pl/cover_HassGetState.yaml Outdated Show resolved Hide resolved
sentences/pl/media_player_HassSetVolume.yaml Show resolved Hide resolved
sentences/pl/_common.yaml Outdated Show resolved Hide resolved
sentences/pl/cover_HassTurnOff.yaml Outdated Show resolved Hide resolved
tests/pl/_fixtures.yaml Outdated Show resolved Hide resolved
tests/pl/light_HassTurnOff.yaml Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft March 6, 2024 18:12
@home-assistant
Copy link

home-assistant bot commented Mar 6, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@Uriziel01
Copy link
Contributor

@witold-gren I've finished the very initial review, I'm sure there is more to fix but even going through it just with just a first glance took almost an hour and because the webUI of GitHub was not happy when jumping around through 6k+ lines of code in the diff 😁 It will have to cut it for now, just ping me when finished fixing/answering and I'll merge it, if I've missed something (and I'm very sure I did missed more bugs) I hope we will fix it on a later iteration as you've proposed. At least that's what I'm hoping for, time will tell 😉

@witold-gren
Copy link
Contributor Author

Like I mentioned, now I'm unavailable until Monday. Big thanks for quick review. However, since this was not added to this version it not make a sens to close it with potential bugs. Now you can work on this PR calmly, let me know when you check everything and I will make corrections. 💪🏻

@witold-gren witold-gren marked this pull request as ready for review March 14, 2024 21:35
@home-assistant home-assistant bot requested review from deejay1 and Uriziel01 March 14, 2024 21:35
@Uriziel01
Copy link
Contributor

@witold-gren Just add missing tests, then let me have a last look and I think we are good to go with this PR 🙂🥳

@witold-gren
Copy link
Contributor Author

@Uriziel01 I improved the expressions related to Media Player so that there are no contains incorrect sentences. Take a look at this place.

Copy link
Contributor

@Uriziel01 Uriziel01 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 for your hard work.

@Uriziel01 Uriziel01 merged commit 8e6ed5a into home-assistant:main Mar 14, 2024
2 checks passed
schizza pushed a commit to schizza/intents that referenced this pull request Mar 16, 2024
…, valve, vacuum, media_player, person, todo, weather and more (home-assistant#1996)
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.

4 participants