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

Add support for DVSPortal (dutch parking) to HA #134289

Draft
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

ChessSpider
Copy link

@ChessSpider ChessSpider commented Dec 30, 2024

Proposed change

Add support of DVSPortal. DVSPortal is a parking system in use by some dutch muncipalities. Like these: https://www.google.com/search?q=inurl%3Advsportal

Documentation PR home-assistant/home-assistant.io#36722

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

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

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:

@ChessSpider ChessSpider requested a review from a team as a code owner December 30, 2024 19:49
Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @ChessSpider

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@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.

@ChessSpider
Copy link
Author

Documentation PR is here home-assistant/home-assistant.io#36722

@ChessSpider ChessSpider marked this pull request as ready for review January 4, 2025 22:41
@MartinHjelmare MartinHjelmare removed the request for review from a team January 4, 2025 22:46
Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

Let's split out the service call for now to keep the PR better scoped

@home-assistant home-assistant bot marked this pull request as draft January 4, 2025 23:45
@ChessSpider ChessSpider marked this pull request as ready for review January 5, 2025 12:20
@home-assistant home-assistant bot requested a review from joostlek January 5, 2025 12:20
@ChessSpider
Copy link
Author

hi @joostlek , im also on discord if you'd like to discuss there. Here in the PR is fine with me as well. thank you for your time and effort!

Note that im aiming for quality scale bronze now, but i do intent to upgrade to higher quality levels once this is merged in.

@ChessSpider
Copy link
Author

hi @joostlek , just to manage my expectations, when do you thnik youll have time for a review? thank you in advance

@joostlek
Copy link
Member

I can review when I have the time, there are 480 or so PRs open so there is quite the queue :)

ha_registered_license_plates: set[str]


type DVSPortalConfigEntry = config_entries.ConfigEntry[DVSPortalRuntimeData]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type DVSPortalConfigEntry = config_entries.ConfigEntry[DVSPortalRuntimeData]
type DVSPortalConfigEntry = ConfigEntry[DVSPortalRuntimeData]

await dvs_portal.close()
raise exceptions.ConfigEntryNotReady("Failed to initialize DVSPortal") from ex

async def async_unload_entry(
Copy link
Member

Choose a reason for hiding this comment

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

Weird indent



async def async_setup_entry(
hass: core.HomeAssistant, entry: DVSPortalConfigEntry
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
hass: core.HomeAssistant, entry: DVSPortalConfigEntry
hass: HomeAssistant, entry: DVSPortalConfigEntry

Comment on lines +53 to +71
try:
# Verify login still works
await dvs_portal.token()

# Setup and init stuff
coordinator = DVSPortalCoordinator(hass, dvs_portal)

entry.runtime_data = DVSPortalRuntimeData(
dvs_portal=dvs_portal,
coordinator=coordinator,
ha_registered_license_plates=set(),
)
await hass.config_entries.async_forward_entry_setups(entry, ["sensor"])

# Check if the first refresh is successful
await coordinator.async_config_entry_first_refresh()
except dvs_exceptions.DVSPortalError as ex:
await dvs_portal.close()
raise exceptions.ConfigEntryNotReady("Failed to initialize DVSPortal") from ex
Copy link
Member

Choose a reason for hiding this comment

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

Only have stuff in the try block that can raise

Comment on lines +25 to +30
class DVSPortalRuntimeData(TypedDict):
"""Typed runtime data for dvsportal."""

dvs_portal: DVSPortal
coordinator: DVSPortalCoordinator
ha_registered_license_plates: set[str]
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need all 3? the client is available via the coordinator and the latter one isnt used

Comment on lines +38 to +45
return {
"default_code": self.dvs_portal.default_code,
"default_type_id": self.dvs_portal.default_type_id,
"balance": self.dvs_portal.balance,
"active_reservations": self.dvs_portal.active_reservations,
"historic_reservations": self.dvs_portal.historic_reservations,
"known_license_plates": self.dvs_portal.known_license_plates,
}
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you use a dataclass? Rather define it here than in types.py

hass,
_LOGGER,
name=f"{DOMAIN}_coordinator",
update_method=self.async_update_data,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
update_method=self.async_update_data,

)
self.dvs_portal = dvs_portal

async def async_update_data(self) -> DVSPortalData:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async def async_update_data(self) -> DVSPortalData:
async def _async_update_data(self) -> DVSPortalData:

"""Fetch data from the DVSPortal API."""
try:
await self.dvs_portal.update()
except Exception as e:
Copy link
Member

Choose a reason for hiding this comment

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

You're only allowed to catch exceptions in the config flow

@home-assistant home-assistant bot marked this pull request as draft January 14, 2025 13:58
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.

2 participants