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

Sitemap Builder #139

Merged
merged 1 commit into from
Sep 18, 2023
Merged

Sitemap Builder #139

merged 1 commit into from
Sep 18, 2023

Conversation

ccutrer
Copy link
Contributor

@ccutrer ccutrer commented Sep 8, 2023

No description provided.

@ccutrer ccutrer force-pushed the sitemap-builder branch 2 times, most recently from a09ba51 to 4c73e6f Compare September 17, 2023 23:30
@ccutrer ccutrer marked this pull request as ready for review September 17, 2023 23:32
@ccutrer ccutrer requested a review from jimtng September 17, 2023 23:32
@ccutrer ccutrer added the enhancement New feature or request label Sep 17, 2023
@jimtng
Copy link
Contributor

jimtng commented Sep 18, 2023

With items.build:

items.build do
  switch_item MySwitch, "My Switch"
end

Should we make sitemaps.build similar to items builder syntax:

    sitemaps.build do
      sitemap "default", "My Residence" do
        text Switch1, "Label"
      end
    end

@ccutrer
Copy link
Contributor Author

ccutrer commented Sep 18, 2023

I thought about that, but with items a name is required, and label is optional, but very common. With sitemaps, both item and label are optional, and it's just as likely that one will be omitted as the other... erego I couldn't choose either as the less common one to place second as a positional parameter. Also, the regular sitemap syntax has both as essentially named parameters, so using keyword arguments for both maps more closely to that.

@jimtng
Copy link
Contributor

jimtng commented Sep 18, 2023

We could use *args and check if it's a Proxy, then it's the item, if it's String, it's the label. The rest (icon, etc) can stay as kwargs. This is just a suggestion / idea, in case you hadn't considered it. Happy to stick to kwargs too.

I also noticed that sitemap takes two non-kwargs, i.e. sitemap "name", "label" whereas openhab syntax uses label= named parameter.

Strictly adhering to the sitemap file syntax may become irrelevant if one day openhab switched to yaml, although this may never actually happen.

@ccutrer
Copy link
Contributor Author

ccutrer commented Sep 18, 2023

heh, yeah, for sitemap, the name is required, so I made it like items. Also, I can't necessarily do item/proxy detection, since String is a valid datatype for the item (same as for Items builder - you can use an Item, or a String).

Copy link
Contributor

@jimtng jimtng left a comment

Choose a reason for hiding this comment

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

Thanks! This is an awesome feature!

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer ccutrer merged commit 1bd2b5f into openhab:main Sep 18, 2023
19 checks passed
@ccutrer ccutrer deleted the sitemap-builder branch September 18, 2023 01:48
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.

2 participants