-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
Improve holidays config form and naming #133663
base: dev
Are you sure you want to change the base?
Conversation
Hey there @jrieger, @gjohansson-ST, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
It's not breaking as it only influence new setups. |
b94bed1
to
ea8ba56
Compare
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
I realised I'm not using the holidays library as intended, and I'm just trying to bypass its features, and make things worse in the end. I should have seen it from my own country:
the same code being used for three different regions. But to take another country as an example:
Austria and Malaysia are indifferently using numerical codes, plain name, abbreviations, and even a single letter when possible. New Zealand is pushing it further with using an English name, along with the Maori one. The thing is, holidays already supports this:
The library accepts and preserves the user choice, and that's that choice I need in the end to name the entity. I'm in favour of user choice, so I'll rework my PR to store the alias in the province configuration. So that in the end, if the user chose "New Zealand" and "Tāmaki-Makaurau" in the config form, the entity will be named "New Zealand, Tāmaki-Makaurau" as requested. And of course, I already started taking all of your suggestions into account. |
We should not store the alias, only the code. And in the case above for New Zealand the resulting entry should be The reason is because these are aliases and prone to be changed etc. So that would result in a config entry to break on startup as example in a future bump of the lib. Also agree some of the aliases are quite weird now looking at it, but I guess that's just how it is and we'll have to live with it. Alternatively we could just use the first alias in the list, but then in the example above with NZ someone might not be soo happy if they really want to have the Maori one. |
Alright, I won't change my PR as much, and users can always manually rename the entity after creation. Ideally, holidays would store both, maybe I can contribute something later. |
We can keep a local variable for the user choice in the config flow and use that for setting the title (the entity is based on the config entry title as well). Then we store the code, but for the user it's set according to the choice they made. It would require a few more lines of code and the selector needs to use the labels (and lookup the code later). I think that's probably the route we should take. |
ea8ba56
to
8f6452a
Compare
Updated my PR with all of your suggestions, but not pushing the "Ready for review" button yet. I'll now look into storing the user choice for the time of the configuration, but keep the ISO code for long-term storage, which is kind of the best of both worlds. |
8f6452a
to
4975fec
Compare
The holidays library is improving over time, let's make use of their data for a more user-friendly experience.
4975fec
to
b605879
Compare
Alright, I think I got it. There's just a compromise, I still have to keep the ISO codes in the valid choices, or an existing config entry wouldn't be validated. So I pushed them to the end of the list, listing the user-friendly names first. I changed my new test to cover another use case: several provinces using the same subdivision code, but let the user pick the one they want. |
The holidays library is improving over time, let's make use of their data for a more user-friendly experience.
Proposed change
I'm changing how the list of provinces is presented to the end user, with user-friendly aliases when available instead of the ISO 3166-2 codes. And then I'm changing how the service is named once created, to use that alias instead of the code. For example, "Germany, Baden-Württemberg" instead of "Germany, BW" (and Baden-Württemberg was listed in the configuration form, along with the other Landers).
Rationale: I cannot speak for the German culture in my example above, but as a (mainland) Frenchman, I didn't understand what Home Assistant was asking me with a list of codes like MQ or RE. So I cheked the holidays library documentation, which wasn't describing them either.
So I contributed to holidays (done), and now I'm using those changes in the holiday integration for a better user experience.
Type of change
Additional information
I'm flagging this as a new feature, because it changes the behaviour of the holiday integration, but it's not really new.
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
.To help with the load of incoming pull requests: