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

[squeezebox] Add new channels for additional tags #14201

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

mhilbush
Copy link
Contributor

@mhilbush mhilbush commented Jan 10, 2023

Resolves #13353

Added channels for the following:

  • albumartist
  • trackartist
  • band
  • conductor
  • composer

Signed-off-by: Mark Hilbush mark@hilbush.com

Signed-off-by: Mark Hilbush <mark@hilbush.com>
@mhilbush mhilbush requested a review from digitaldan as a code owner January 10, 2023 21:53
@jlaur jlaur added the enhancement An enhancement or new feature for an existing add-on label Jan 10, 2023
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Thank you. Looks simple and good to me! I have added comments for a request to use camelCase for the two multi-worded new channels.

@jlaur jlaur changed the title [squeezebox] New channels for additional tags [squeezebox] Add new channels for additional tags Jan 10, 2023
@mhilbush
Copy link
Contributor Author

Ha ha. I actually started down the camelCase path. But I changed it to be consistent with the tag naming convention in the LMS, and also because I didn't want to change remotetitle and coverartdata to camelCase, as that would be a breaking change.

I admit that the binding is a bit inconsistent in how the channels are named. Some are camelCase and some are not.

But I'm happy to change the new channels to camelCase if that's the recommendation. Just let me know.

@jlaur
Copy link
Contributor

jlaur commented Jan 11, 2023

Ha ha. I actually started down the camelCase path. But I changed it to be consistent with the tag naming convention in the LMS, and also because I didn't want to change remotetitle and coverartdata to camelCase, as that would be a breaking change.

Correct, it's a pickle staying consistent and conforming to naming conventions and at the same time without breaking backwards compatibility. 🙂 However, one thing we shouldn't do is adopting naming conventions from the API's we implement in bindings. We have our own, making openHAB consistent across bindings and being API agnostic.

I admit that the binding is a bit inconsistent in how the channels are named. Some are camelCase and some are not.

For consistency we will have to choose one of the two (camelCase or lowercase). See below.

But I'm happy to change the new channels to camelCase if that's the recommendation. Just let me know.

Yes, that's the recommendation. And since it's already in use for some of the channels, this would be the preferred way going forward. See also https://community.openhab.org/t/naming-convention-for-channels/128841/10.

Had all the existing channel been lowercased (or underscore_separated or dash-separated), the situation would be different. Here I would recommend to prioritize consistency higher than the current recommendation.

Signed-off-by: Mark Hilbush <mark@hilbush.com>
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM

@jlaur jlaur merged commit 858ff4b into openhab:main Jan 11, 2023
@jlaur jlaur added this to the 4.0 milestone Jan 11, 2023
@mhilbush mhilbush deleted the squeezebox-new-channels branch January 11, 2023 13:35
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
* New channels for additional tags

Signed-off-by: Mark Hilbush <mark@hilbush.com>
renescherer pushed a commit to renescherer/openhab-addons that referenced this pull request Mar 23, 2023
* New channels for additional tags

Signed-off-by: Mark Hilbush <mark@hilbush.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[squeezebox] Add channels for composer, band and conductor tags.
2 participants