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

Hass model areas #542

Merged
merged 19 commits into from
Dec 9, 2021
Merged

Hass model areas #542

merged 19 commits into from
Dec 9, 2021

Conversation

parkerman92
Copy link
Contributor

Breaking change

Proposed change

Fixes an issue in APIv2 where area is populated with the area id instead of the name (which I think is incorrect)
Adds area name to the new hass model api.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to an existing integration)
  • 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][dev-checklist]
  • The code compiles without warnings (code quality chek)
  • Tests have been added to verify that the new code works.

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

var areas = await _hassClient.GetAreas().ConfigureAwait(false);
var areaDict = areas?.ToDictionary(k => k.Id!, v => v);

_latestAreas.Clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of Clearing the dictionary and re-loading you could create a whole new dictionary, load it and then re-assign the private field to the new dictionary when it is fully loaded. This will prevent any possible concurrency issue between clearing and re-filling it. You then don't even need a ConcurrentDictionary because it is only ever written to initially by one thread

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added - Commit

return _latestAreas.TryGetValue(entityId, out var result) ? result : null;
}

public void Dispose()
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I like to put Dispose() last

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved

/// </summary>
/// <param name="entityId"></param>
/// <returns></returns>
HassArea? GetArea(string entityId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the HassArea class from the HassClient? I prefer not to expose that in the HassModel public API. HassModel currenty only uses HassClient internally.

I was thinking of a design where IHaContext returns some more metadata about an Entity in the form of an EntityMetaData object that has Device, Area etc. That will however be a bit more complicated as we need to design such a structure which is basically why this Area was not implemented yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed on discord. Refactored to use simple class and not expose the HassClient models.

@FrankBakkerNl
Copy link
Contributor

I really like this It is almost exactly what I had in mind for this.
Just the public API of IHaContext I would not like to depend on HassClient objects Lets discuss on Discord on how exactly to make the API

@parkerman92
Copy link
Contributor Author

Cheers for the feedback @FrankBakkerNl any other suggestions let me know 👍

@helto4real
Copy link
Collaborator

Thank you for the PR!

parkerman92 and others added 4 commits December 9, 2021 22:21
Co-authored-by: Frank Bakker <FrankBakkerNl@users.noreply.github.com>
Co-authored-by: Frank Bakker <FrankBakkerNl@users.noreply.github.com>
Co-authored-by: Frank Bakker <FrankBakkerNl@users.noreply.github.com>
@FrankBakkerNl FrankBakkerNl merged commit edcf04c into net-daemon:dev Dec 9, 2021
Ikcelaks pushed a commit to Ikcelaks/netdaemon that referenced this pull request Dec 23, 2022
* Fix return area id instead of area name.

* Fix UT for area id vs name.

* Create new area cache

* Add area cache and expose get area to HaContext implementation and interface

* Update entity to get area name from context

* Update DI

* Add tests for area cache and entity wrap

* tidy and refactor init call to cache

* rebuild list locally then reasign

* move dispose to last

* add area class

* Add mapper from HassArea -> Area

* Refactor removing HassClient objects

* update entity GetAreaFromEntityId

* update UT GetAreaFromEntityId

* Update src/HassModel/NetDeamon.HassModel/Entities/Area.cs

Co-authored-by: Frank Bakker <FrankBakkerNl@users.noreply.github.com>

* Update src/HassModel/NetDeamon.HassModel/Internal/HassObjectMapper.cs

Co-authored-by: Frank Bakker <FrankBakkerNl@users.noreply.github.com>

* Update src/HassModel/NetDeamon.HassModel/Internal/HassObjectMapper.cs

Co-authored-by: Frank Bakker <FrankBakkerNl@users.noreply.github.com>

* remove area from m3app

Co-authored-by: Frank Bakker <FrankBakkerNl@users.noreply.github.com>
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.

None yet

3 participants