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

Improve holidays config form and naming #133663

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

bors-ltd
Copy link

@bors-ltd bors-ltd commented Dec 20, 2024

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

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

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

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @jrieger, @gjohansson-ST, mind taking a look at this pull request as it has been labeled with an integration (holiday) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of holiday can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign holiday Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@gjohansson-ST
Copy link
Member

It's not breaking as it only influence new setups.
Move that part of the description (friendly name instead of code when exists) to the proposed change section.

@bors-ltd bors-ltd force-pushed the holidays_aliases branch 2 times, most recently from b94bed1 to ea8ba56 Compare December 20, 2024 15:46
homeassistant/components/holiday/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/holiday/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/holiday/config_flow.py Outdated Show resolved Hide resolved
tests/components/holiday/test_config_flow.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft December 20, 2024 16:50
@home-assistant
Copy link

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.

@bors-ltd
Copy link
Author

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:

 'GES': ['Alsace', 'Champagne-Ardenne', 'Lorraine'],

the same code being used for three different regions.

But to take another country as an example:

{'1': ['Burgenland', 'Bgld', 'B'],

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:

>>> nz_english = country_holidays('NZ', 'Auckland')
>>> nz_english.subdiv
'Auckland'
>>> nz_maori = country_holidays('NZ', 'Tāmaki-Makaurau')
>>> nz_maori.subdiv
'Tāmaki-Makaurau'

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.

@gjohansson-ST
Copy link
Member

gjohansson-ST commented Dec 21, 2024

We should not store the alias, only the code. And in the case above for New Zealand the resulting entry should be New Zealand, Auckland, Tāmaki-Makaurau (or something similar).

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.

@bors-ltd
Copy link
Author

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.

@gjohansson-ST
Copy link
Member

gjohansson-ST commented Dec 21, 2024

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.

@bors-ltd
Copy link
Author

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.

The holidays library is improving over time, let's make use of their
data for a more user-friendly experience.
@bors-ltd
Copy link
Author

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.

@bors-ltd bors-ltd marked this pull request as ready for review December 21, 2024 11:43
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.

Improvement to holidays config form
2 participants