search

kalkih / mini-graph-card

Minimalistic graph card for Home Assistant Lovelace UI
MIT License
2.87k stars 231 forks source link

docs: Update Readme: explaining addressing data in "tree" structures #1001

Closed ildar170975 closed 10 months ago

ildar170975 commented 10 months ago

Docs for https://github.com/kalkih/mini-graph-card/pull/996

ildar170975 commented 10 months ago

These sensors may be used for testing:

sensor:
  - platform: random
    name: test_object_data_random_value_1
    <<: &ref_random_settings
      minimum: 0
      maximum: 100
      unit_of_measurement: ''
  - platform: random
    name: test_object_data_random_value_2
    <<: *ref_random_settings
  - platform: random
    name: test_object_data_random_value_3
    <<: *ref_random_settings

template:
  - sensor:
      - name: testing_object_data
        state: 999
        attributes:
          dict_attribute: >-
            {% set DICT = {
              'value_1': states('sensor.test_object_data_random_value_1'),
              'value_2': states('sensor.test_object_data_random_value_2'),
              'value_3': states('sensor.test_object_data_random_value_3')
              } -%}
            {{DICT}}

      - name: testing_object_data_list
        state: 999
        attributes:
          list_attribute: >-
            {% set LIST = namespace(elements=[]) -%}
            {%- for i in range(3) -%}
              {%- set COEFF = i+1 -%}
              {%- set DICT = {
                'value_1': states('sensor.test_object_data_random_value_1')|float(default=0) * COEFF,
                'value_2': states('sensor.test_object_data_random_value_2')|float(default=0) * COEFF,
                'value_3': states('sensor.test_object_data_random_value_3')|float(default=0) * COEFF
              } -%}
              {%- set LIST.elements = LIST.elements + [DICT] -%}
            {%- endfor -%}
            {{LIST.elements|list}}
akloeckner commented 10 months ago

Hmm... A few general remarks up front:

Sorry for these general remarks. Right now, I have the impression we can do better: more concise and with more clarity. But I am more of a technical guy, so from user's perspective I might not have the right tone in mind.

Edit: and of course, we should include screenshots in the repository itself.

ildar170975 commented 10 months ago

Don't you think the (very brief, yes) hint in the description of the attribute option is sufficient? If not, we should link the new section from there. And be sure that the new section clarifies that this behaviour only applies to the attribute option. And ideally have the examples in both sections the same, or similar.

Not sure what you mean. Could you provide a short example of this hint? There are 2 cases described - dictionary & list, for both a general structure is provided: -- entity_id.dict_attribute.sub_attribute -- entity_id.list_attribute.index.sub_attribute

I would not make the examples too "big". They should be minimal. One graph should be sufficient. Ideally using an entity that everyone has or can create easily. How about weather.home? It already has a list of forecasts. So, we could use attribute: forecast.0.temperature, if that works.

Was thinking about using a weather entity - but have some doubts:

  1. The point is that a weather entity may have forecast data. I am not sure that all used integrations do provide forecast. Myself using Metno, Gismeteo, OpenWeatherMap - all of them have forecast, but may be some integration does not have it? I am not expert here, just my guess.
  2. Some users (including me) exclude weather entities from Recorder - due to that rather huge forecast data, so it is impossible to build graphs. May be I am doing smth wrong, may be storing this domain is optimized in DB. Asked in Community about this, may post here a feedback. Update: yes, seems these cumbersome data are stored like other attributes.
  3. So, if storing weather entities is not recommended, then IMHO we should not mention this in our examples.
  4. Also in general, I do not think that someone really is interested in building graphs with forecast data. Users are mainly interested in smth like "how current temperature is changing" (myself use template sensors based on weather attributes & included in Recorder). As for forecast, some users asked about "build graphs for forecast with time/dates on X axis" - which is currently not possible.
  5. From other side, some users have REST-like sensors which may have lists & dictionaries as attributes (but here again same question about storing in DB). Anyway, it is up to a user if he wants to declutter his DB with useless data or not... The mini-graph-card does provide a way to deal with these complex structures - and we should explain how to use it.

I am surprised that indexing a list works with our implementation, because we actually retrieve list['1'] and not list[1].

Should we test this feature (accessing arrays) more? Probably it does not work as I believe?

I think "long state" is a bit misleading as wording. The state is a string always. We're talking about attributes. And they are not "long", I would say, but rather "structured" or a "dictionary". But if we use a real-world example, the wording will naturally change

Agree! If we decide to keep these examples (i.e. not using weather) - I should rename it to smth like dict_data or list_data. What do you think?

  • If it really is that easy, I would also only use one example, involving a list, e.g. the forecasts. And state something like "Note, how lists can be accessed using indices such as forecast.0."

Good point. But some people need simple examples (for educational purpose too) and they tend to learn things in a "simple -> complex" way. That is why I propose to give a separate example for a dictionary ("simple") and a separate example for a list ("complex") (although it covers the 1st example too). It is just a matter of explaining things. See, we have plenty of same questions in Community, and some of them are asked since people do not see a required example in Docs. And ofc we cannot cover all cases in Docs...

But I am more of a technical guy, so from user's perspective I might not have the right tone in mind.

It is ok, I am a former cruise missiles dev ))), together we may create nice docs.

we should include screenshots in the repository itself.

Does it mean that "jpg" files should be stored along with a code & readme.md? Just checked - have not found any images there.

paqpaqpaq commented 10 months ago

@ildar170975 I would very much like some elaboration on the weather "tree" data. If a certain sensor has an attribute with say 25 forecast points, to be divided over the 'future' 2 hours, like so:

precip:
  - 0
  - 0
  - 0,25
  - 0,30
  - 0,10
  - 0
  - 0
  - 0,95
  - 1,2
  - 0
  - 0
etc
akloeckner commented 10 months ago

Don't you think the (very brief, yes) hint in the description of the attribute option is sufficient?

Not sure what you mean. Could you provide a short example of this hint?

I had asked to document the new feature in a very brief way here: https://github.com/kalkih/mini-graph-card/blob/50b172ef463ef7a2dc8c5def6801c55301ed7831/README.md?plain=1#L127

How about weather.home? It already has a list of forecasts. So, we could use attribute: forecast.0.temperature, if that works.

I just checked, and it does not work. Interestingly, weather forecasts are not real attributes, but provided by a separate API. That's maybe worth noting in your community thread as well, because those forecasts are not stored in the database 🤷 (see https://developers.home-assistant.io/docs/core/entity/weather/#weather-forecasts). I double-checked in my database and they are not there. So, we can forget about using weather entities. And there don't seem to be any other default integrations with tree-like attributes (see https://www.home-assistant.io/integrations/default_config/).

I am surprised that indexing a list works with our implementation, because we actually retrieve list['1'] and not list[1].

Should we test this feature (accessing arrays) more? Probably it does not work as I believe?

No, I just did and it does work. I am still stumbled. But why complain. 😆

I think "long state" is a bit misleading as wording. The state is a string always. We're talking about attributes. And they are not "long", I would say, but rather "structured" or a "dictionary". But if we use a real-world example, the wording will naturally change

Agree! If we decide to keep these examples (i.e. not using weather) - I should rename it to smth like dict_data or list_data. What do you think?

Yes,that sounds good!

  • If it really is that easy, I would also only use one example, involving a list, e.g. the forecasts. And state something like "Note, how lists can be accessed using indices such as forecast.0."

Good point. But some people need simple examples (for educational purpose too) and they tend to learn things in a "simple -> complex" way. That is why I propose to give a separate example for a dictionary ("simple") and a separate example for a list ("complex") (although it covers the 1st example too). It is just a matter of explaining things. See, we have plenty of same questions in Community, and some of them are asked since people do not see a required example in Docs. And ofc we cannot cover all cases in Docs...

Ok, I agree. Let's explain more details up-front and expect less questions afterwards. :+1:

It is ok, I am a former cruise missiles dev ))), together we may create nice docs.

😱 Oh... I'm afraid now. 😉 🚀

By the way: I just noticed your PR is #1001.

we should include screenshots in the repository itself.

Does it mean that "jpg" files should be stored along with a code & readme.md? Just checked - have not found any images there.

You're right. But that's a flaw of the documentation. I would say, all documentation (including images) should be stored in the repository itself. Apparently, the default of the online editor within GitHub is different (see https://github.com/orgs/community/discussions/23840). But let's stick to the current practice for now. I'll open another issue to import all documentation images into the repository.

I'll try and suggest some concrete changes next!

ildar170975 commented 10 months ago

I had asked to document the new feature in a very brief way here:

Well, this short description is quite sufficient imho. But let's make the "sub-attribute" a hyperlink to the proposed paragraph "Addressing data in complex structures"? Then a user may go to the link and see a detailed description of what a "sub-attribute" is.

So, we can forget about using weather entities

Well, some integrations still may provide complex data stored in DB (not to mention user-defined template sensors). And we have a ready mechanism to use these data for building graphs.

Yes,that sounds good!

Done, I edited examples & testing code.

akloeckner commented 10 months ago

Sorry, if I messed up this PR. :-) I have opened this PR to your repository to suggest changes: https://github.com/ildar170975/mini-graph-card/pull/1

akloeckner commented 10 months ago

But let's make the "sub-attribute" a hyperlink to the proposed paragraph "Addressing data in complex structures"?

Good idea! I'd include "(attr1.attr2)" in the link, too.

ildar170975 commented 10 months ago

Sorry, if I messed up this PR. :-) I have opened this PR to your repository to suggest changes: ildar170975#1

Well, I will try to ADD your changes in to THIS my PR, ok ?

ildar170975 commented 10 months ago

@akloeckner Can I merge it now? Shall I just press "merge pull req" button (w/o any "rebase" etc)? Asking 'cause never did it so far))

akloeckner commented 10 months ago

Can I merge it now?

Yes, the honour is yours. :-)

The green button should read "squash and merge", which means a single commit with the agreed improvements will be added to the repository in your name. That's what we want, because the discussion does not really matter in the end. (Clicking the arrow-down-button will give you other options. But you should not need them.)

If you click the green button, two input fields will open up for you to enter a headline and a description of the commit to be generated. I would just leave everything as it is. Maybe remove the "update readme" part, just jbecause I don't like two colons in the headline. ;-)

Click "confirm" and... Congratulations to your first merge!

akloeckner commented 10 months ago

Hmm, somehow "squash and merge" doesn't seem to have worked. But never mind: the improvements are there. That's what counts. I'm curious what the automated release script will make of it. :-)

github-actions[bot] commented 5 months ago

:tada: This PR is included in version 0.12.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: