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

Streamline api #4

Merged
merged 15 commits into from
Jan 29, 2024
Merged

Conversation

alexschwantes
Copy link
Contributor

This is somewhat of an experiment.

I saw that there was duplicate syncing being done after an add/update/delete.

eg. when a new todo is added in HA, the call heirachy goes something like:

  • todo.async_create_todo_item()
    • api.async_create_todo_item
      • which performs a _keep.sync
    • async_write_ha_state()
    • coordinator.async_request_refresh
      • which then calls async_update_data which calls api.async_sync_data which calls _keep.sync (again)

The proposed changes attempt to contain sycning to google keep to one centralised place api.async_sync_data(), and further, only the configured lists are synced.

I think that the async_write_ha_state() call is also not required as homeassistant.components.todo._async_write_ha_state() seems to handle updating the state.

Additionally coordinator.async_request_refresh() is supposed to batch requests together, however it proved to be unreliable in my testing, often resulting in checked items remaining in the todo part of the list. So I changed it to coordinator.async_refresh() instead. coordinator.async_refresh() actually ends up calling the coordinator's update_method which is now responsible for syncing to google keep.

Unfortunately, debugging the project on windows doesn't work with breakpoints for tests. And I'm at a bit of a loss when it comes to getting the mock/patching working. The specific failing tests are expected given the scope of changes.

Anyway, have a look and see what you think. Probably the best way to test it is to install it and see if it works as before.

@alexschwantes
Copy link
Contributor Author

hmm it seems vscode + python = poo, when it comes to pre-commit hooks/linting working out of the box ;(
Anyway, have a look and give me some feedback and I can fix up the linting (although might need some help with the tests).

@watkins-matt
Copy link
Owner

Yes, definitely, I'll take a look at it. I'm on vacation right now though so I will probably not be able to until next week or so.

@watkins-matt
Copy link
Owner

Tested it out and everything seems to be working correctly. Just seems to be a matter of fixing the tests. I'll try to find some time to dig in more to see what's going on.

test_async_create_todo_item just seems to be failing because it is expecting the list_id to be returned by async_create_todo_item.

@watkins-matt
Copy link
Owner

I pushed a fix to the tests for test_create_todo_item and test_async_create_todo_item.

For test_create_todo_item, we aren't directly updating the coordinator anymore, which was causing the assertion to fail because the data wasn't present in the coordinator.

I'm not extremely familiar with the inner-workings of the Home Assistant code, but I'm assuming that the coordinator data will get updated in coordinator.async_refresh. Because mock_api and mock_coordinator are both mocks and not actual objects, I had to add side effects for mock_api.async_create_todo_item and mock_coordinator.async_refresh to reflect what happens in those functions.

The changes for test_update_todo_item and test_delete_todo_items should be similar to test_create_todo_item.

@watkins-matt watkins-matt merged commit cfc25c9 into watkins-matt:main Jan 29, 2024
2 checks passed
@watkins-matt
Copy link
Owner

Merged, thank you.

@watkins-matt watkins-matt added the enhancement New feature or request label Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants