openhab-scripters / openhab-helper-libraries

Scripts and modules for use with openHAB
Eclipse Public License 1.0
88 stars 70 forks source link

Add Metadata classes #88

Closed CrazyIvan359 closed 5 years ago

CrazyIvan359 commented 5 years ago

As I mentioned, metadata classes!

@openhab-5iver I don't have write access on #61 so I couldn't push to that branch. I figured since mine differs so much from that one I would start my own branch and PR for it.

@mjcumming Credit where credit is due Michael, I started writing this module with your code in #61 as a reference point and it was a great help as I had not done anything with metadata before this. I found your class to be rather raw in that you always manually read and write to the registry. I moved to a more hands off approach where the class immediately loads when created and writes when any changes are made. My metadata_namespace class loosely resembles your class, and I added another simple class to allow managing multiple namespaces of a single item from one object.

 

and a provider should be used (which I think I wrote already).

I forgot you had mentioned this until I was looking at the other PR to compose this message. I don't see anything metadata in the codebase. Presently I am not using a Provider, but I found the code in OH Core. I will look into this more, though at present I'm not entirely sure it's necessary for the purposes of this class. As I understand it, the Provider allows OH to know what attributes a namespace should have, but that seems beyond the scope of this module as it stands. I could create a Provider that would allow adding namespace definitions from JSR223 code, this would be useful for what I am getting into as it would allow changes to the namespaces from PaperUI.

CrazyIvan359 commented 5 years ago

Presently I am not using a Provider, but I found the code in OH Core. I will look into this more, though at present I'm not entirely sure it's necessary for the purposes of this class. As I understand it, the Provider allows OH to know what attributes a namespace should have, but that seems beyond the scope of this module as it stands. I could create a Provider that would allow adding namespace definitions from JSR223 code, this would be useful for what I am getting into as it would allow changes to the namespaces from PaperUI.

So I dug around in openhab-core and understand Providers/Registries a little more now. They do not provide templates, but I'm still not sure they are the right approach for this class. If I use a custom provider we would only have access to metadata that had been created in Jython code. The MetadataRegistry seems to implement a built in ManagedMetadataProvider that looks after any add/update/remove requests and saves to the jsondb. While the custom provider approach makes sense for most use cases when thinking of items and thing creation in scripts, I don't think its the best approach for metadata.

mjcumming commented 5 years ago

@CrazyIvan359 changes to metadata look great.

5iver commented 5 years ago

Once lucid is merged, lets get this one merged and then go after the documentation.

mjcumming commented 5 years ago

Let me know what is needed

CrazyIvan359 commented 5 years ago

@openhab-5iver I think this is ok to merge, let me know if you need me to make any changes though

CrazyIvan359 commented 5 years ago

This looks well written. Have you fully tested it though? I had to do a PR recently for core.date, due to a number of errors we came across in the lucid migration.

I have not fully tested it yet, no. I also forgot that I never did tests of all of core.date either. I don't have a working dev setup for openHAB + Jython right now though, thanks to the ESH merge, so testing is a bit difficult for me. Do you know of a way to apt install any build of OH? I can only see about a dozen recent builds and don't think I found an archive of build packages anywhere. Even better would be your branch that fixes all of the import names post ESH merge. I haven't had much time to work on a computer in the last couple weeks, though I might have enough time to test tonight.

When I wrote this, I did use it to read and correctly type all types it recognises. It reads value and the config array correctly. I only used the namespace class, not the item class in those tests.

Some simple examples at the top of the Jython-Modules.md entry would be helpful... like adding, reading, removing metadata from an Item.

Good idea. I'll clean it up.

I will finish up the page I'm writing in the docs and get to work on this

CrazyIvan359 commented 5 years ago

I've made some of the changes, waiting on your input on others. Also cleaned up a few things, added some sanity checks on namespace names, checking for item existing, and added None to recognised types.

I did not have time to setup a test env to do further tests yet and may not have time to work on this tomorrow, so I've pushed the changes here in case you wanted to work on it.

CrazyIvan359 commented 5 years ago

@openhab-5iver was this just waiting on testing?

5iver commented 5 years ago

Documentation, mainly examples, and testing. It was difficult for me to use it. I also question why we'd want the caching, which complicates things. This one is next, after the doc rewrite.

mjcumming commented 5 years ago

Documentation, mainly examples, and testing. It was difficult for me to use it. I also question why we'd want the caching, which complicates things. This one is next, after the doc rewrite.

I have kept with the implementation I did a pull request for - its not as versatile but its simpler and fits better with my needs.

CrazyIvan359 commented 5 years ago

Well the caching is for simplicity I suppose, reading from the registry is a bit involved. Also consider the life-cycle of the class, it will only be around for the time it takes a rule to run.

mjcumming commented 5 years ago

Well the caching is for simplicity I suppose, reading from the registry is a bit involved. Also consider the life-cycle of the class, it will only be around for the time it takes a rule to run.

I depends on your use case - I have scripts that constantly refer to metadata and change it as required. Caching would introduce some potential issues that would be hard to track down.

CrazyIvan359 commented 5 years ago

This module has undergone major changes and a new PR has been made (#167)