openhab-scripters / openhab-helper-libraries

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

Initial submittion of a reusable hysteresis function. #222

Closed rkoshak closed 3 years ago

rkoshak commented 4 years ago

Solves Issue #221.

I'm not certain I did everything right. I wanted to start with something simple to use as my initial PR. This is my first code PR (beyond what I do to my own repos) so I'm sure I messed something up.

I tried to use what is in place as a guide for how to format docstrings and comments but there appear to be inconsistencies across the repo. I chose what seemed to make sense.

I assume the HTML files get generated by the Sphinx file, right?

Signed-off-by: Rich Koshak rlkoshak@gmail.com

rkoshak commented 4 years ago

Oops, I already see I messed up. I'm moving the files to the proper locations.

CrazyIvan359 commented 4 years ago

You beat me to it!

Let me know about the inconsistencies in the format suggestions so we can clean them up.

rkoshak commented 4 years ago

Let me know about the inconsistencies in the format suggestions so we can clean them up.

It's more inconsistencies with the formatting of the docsctrings. For example, core.date.to_java_zoneddatetime has a very thorough docstring listing the arguments, returns, and exceptions raised. Some functions in core.items, and all of core.jsr223 completely lack a dockstrings. core.log.log_traceback has lots of good information but isn't structured the same as core.date.to_java_zoneddatetime. Because it's a decorator maybe fn is understood what it is.

I was kind of looking for a standard (e.g. use Args: instead of Arguments) but I couldn't figure out a real pattern.

CrazyIvan359 commented 4 years ago

Since this is so small, you should put everything in __init__.py or have everything in a file called hysterisis.py instead of the folder.

CrazyIvan359 commented 4 years ago

Different modules have been written by different people. Also as you say, different purposes need a different format sometimes.

As for the Args Arguments thing, either can be used. I don't know if we have decided on one or the other, but perhaps we should. I would lean towards Arguments

rkoshak commented 4 years ago

What about the tests? Should those be included? If yes, is that the right place for them?

CrazyIvan359 commented 4 years ago

That would be a question for Scott, I've only done ad hoc tests for my contributions so far.

rkoshak commented 4 years ago

Here's a question. Would it be better to submit just one library folder with all the DPs and examples from the forum like this that I plan on developing, or would it be better to create separate submissions?

My overall intent is to create a series of building blocks that implement stuff that users will commonly need to implement, like the design patterns and generic presence detection and such.

Putting them all in one library may keep it more manageable but then users will get everything. Having them separated let's users pick and chose but will make the community list much much longer.

I would consider this submission to be a part of the combined library, if we decide that's the way to go.

CrazyIvan359 commented 4 years ago

I think having them all separate would be the way to go, otherwise they end up rather hidden.

5iver commented 4 years ago

Rich, I'm usually much quicker to respond to PRs. My time has been limited, so I'm sorry for the delay. I'm thrilled to see you getting some things submitted and hate to be holding you up. Every spare minute I have has been going into getting Area Triggers and Actions polished and documented. In the process, I've been thinking through and testing some things related to what you're asking.

Community is great for packages and scripts that users don't need to modify... where user config can be pulled from configuration.py. Things get a bit ugly where there are example modules or scripts provided that users would need to customize. Nothing in the community (or core) directories should ever be modified by the user, since it could get overwritten by an update. Community packages and scripts that are mainly examples could potentially use the personal directories, but this gets messy and I don't see this working once community gets packaged for distribution. ideAlarm has some of this. I'm currently doing the same with Area Triggers and Actions, but I don't like it and may put a lot of it into Script Examples, which also needs to be renamed so that we can put more than scripts in there.

If building something that is fully contained, then put it in community. I recently got Mode (Time of Day) moved back into community. Everything else should go into Script Examples. Use the personal directory structure, so that users can just copy/paste things from it.

Would it be better to submit just one library folder with all the DPs and examples from the forum like this that I plan on developing, or would it be better to create separate submissions?

These should definitely go into separate directories and in separate PRs too. Remember, the files can be scattered all over the repo but we can make it seamless in the documentation. People will be reading about it in the docs and also unzipping a directory and browsing the files.

What about the tests? Should those be included? If yes, is that the right place for them?

We need to build out at least some regression tests for everything in core and a script to kick them all off. IIRC, the core.testing module is not fully functional after the 2.3 API change and I still have not gone through it. I planned to tear into and expand it at some point and then use it to build out the tests. For now, just post them in the comments of the PR. If you'd like to get things started though, create...

Tests
    |_ community
        |_ hysteresis
            |_ your_tests.py

I was kind of looking for a standard (e.g. use Args: instead of Arguments) but I couldn't figure out a real pattern.

The coding guidelines have not been documented yet. Do you're best, ask questions, and I'll point things out in the review. The core.metadata and core.date have gotten the most attention. The original files from Steve had little to no docstrings and they've only been getting built out since the rework of the docs. You're welcome to start filling things in. :smile:

I assume the HTML files get generated by the Sphinx file, right?

Yes. It's not that hard to get the docs built, but it's tricky when new things are added and the build needs to be tweaked. I've been taking care of the docs and will point things out in the review if the build of the pages doesn't work.

rkoshak commented 4 years ago

hate to be holding you up.

No worries. I know you are busy. I'm just dipping my toe into this right now and have plenty to keep me busy.

Community is great for packages and scripts that users don't need to modify.

That's my intent. All the code I'm writing is written such that if there is any configuration necessary, it will be done through configuration.py or through Items/Groups. I intend the code itself to not be touched. And in many cases this drives to creating modules instead of scripts (e.g. the Gatekeeper DP I've implemented as a module, my version of Time of Day just has two values to update in configurations.py, etc.).

definitely go into separate directories and in separate PRs too. 👍

I've started on a good footing then.

the core.testing module is not fully functional after the 2.3 API change

That's good to know. I was going to try converting my tests to use that API this week. I'll hold off on that for now. I'll move them to the tests file like you recommend. I prefer to keep them with the code as often (and I may be weird in this) I learn more from the tests than I do from the docs. :-)

I'll fix the tests and then await the review. Take you time. There is nothing burning down while this PR waits. I've plenty of other stuff to work on. But I am waiting to submit any new PRs until I learn my lessons from this one. ;-)

rkoshak commented 4 years ago

All raised issued have been addressed. This PR is ready for review.