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

[items & things] Add toString overrides & Clean-Ups #198

Merged
merged 7 commits into from
Dec 26, 2022

Conversation

florian-h05
Copy link
Contributor

@florian-h05 florian-h05 commented Dec 18, 2022

Reference #197 (fixes the JS library side).

To Dos

  • Regenerate type definitions
  • Update CHANGELOG

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
They were exported but undocumented so no one should have been using
them and they are really advanced functionality.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05 florian-h05 added the bug Something isn't working label Dec 18, 2022
@florian-h05
Copy link
Contributor Author

@jpg0 Can I remove thing builder and channel builder here?

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05 florian-h05 requested a review from jpg0 December 20, 2022 17:05
@florian-h05 florian-h05 changed the title [Do NOT merge] [items & things] Add toString overrides & Clean-Ups [items & things] Add toString overrides & Clean-Ups Dec 20, 2022
@jpg0
Copy link
Collaborator

jpg0 commented Dec 20, 2022

@jpg0 Can I remove thing builder and channel builder here?

Are they used? I don't mind whether they go or stay, I suppose it depends on the purpose of the library. I used them to define and register things and channels in scripts (and then refer to them from other scripts). I did this to move all my configuration into JS, and then could reference everything via JS imports instead of name strings.

However if this library is limited to rules based on existing definitions, then this functionality should be removed.

Maybe @digitaldan has an opinion?

@digitaldan
Copy link
Contributor

Maybe @digitaldan has an opinion?

I would error on the side of simplicity. Since it's undocumented and probably unused by most, i think removing it makes sense.

@florian-h05
Copy link
Contributor Author

So we agree that we remove that?

Can one of you please review here (although the changes apart from the removal are minimal).

@florian-h05 florian-h05 added this to the to be released milestone Dec 22, 2022
@florian-h05
Copy link
Contributor Author

florian-h05 commented Dec 23, 2022

@jpg0 @digitaldan Can you please review here? You can ignore the *.d.ts files, these type definitions are auto generated and only required for auto-completion, not for the library's functionality itself.
If you approve, please let me merge - I have to adjust the commit message.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Copy link
Contributor

@digitaldan digitaldan left a comment

Choose a reason for hiding this comment

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

LGTM!

@florian-h05 florian-h05 merged commit 3ba6673 into openhab:main Dec 26, 2022
@florian-h05 florian-h05 deleted the items-things-tostring branch December 26, 2022 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants