-
-
Notifications
You must be signed in to change notification settings - Fork 69
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
Initial submittion of a reusable hysteresis function. #222
Conversation
Signed-off-by: Rich Koshak <[email protected]>
Oops, I already see I messed up. I'm moving the files to the proper locations. |
You beat me to it! Let me know about the inconsistencies in the format suggestions so we can clean them up. |
… license info. Signed-off-by: Rich Koshak <[email protected]>
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. |
Since this is so small, you should put everything in |
Different modules have been written by different people. Also as you say, different purposes need a different format sometimes. As for the |
What about the tests? Should those be included? If yes, is that the right place for them? |
That would be a question for Scott, I've only done ad hoc tests for my contributions so far. |
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. |
I think having them all separate would be the way to go, otherwise they end up rather hidden. |
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
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.
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...
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. 😄
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. |
No worries. I know you are busy. I'm just dipping my toe into this right now and have plenty to keep me busy.
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.).
I've started on a good footing then.
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. ;-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything in the community package should be a package. However, this would be worthwhile in core.utils.
Community/Hysteresis/automation/lib/python/community/hysteresis.py
Outdated
Show resolved
Hide resolved
Community/Hysteresis/automation/lib/python/community/hysteresis.py
Outdated
Show resolved
Hide resolved
Community/Hysteresis/automation/lib/python/community/hysteresis.py
Outdated
Show resolved
Hide resolved
Community/Hysteresis/automation/lib/python/community/hysteresis.py
Outdated
Show resolved
Hide resolved
Community/Hysteresis/automation/lib/python/community/hysteresis.py
Outdated
Show resolved
Hide resolved
Community/Hysteresis/automation/lib/python/community/hysteresis.py
Outdated
Show resolved
Hide resolved
|
||
return rval | ||
|
||
# Hysteresis test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take this out and put in into a comment in the PR or add to a new Tests directory off of the root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've moved these, but put the Tests directory under .github
. Please move it up to the root (same level as Core and Community).
I'm rethinking this directory structure though. I think it would be better to put the tests in packages, like /Tests/automation/lib/python/tests/core/utils.py
. The ones for hysteresis would go in there and we can add in others for the rest of utils. Then we can use a script to load the module and execute the tests. Still just drawing in the sand though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense for Communty to put the tests with the modules? For example:
Modules: Community/Gatekeeper/automation/lib/python/community/gatekeeper/gatekeeper.py
Scripts:
Community/Gatekeeper/automation/jsr223/python/community/gatekeeper/gatekeeper_script.py
Tests: Community/Gatekeeper/automation/test/python/gatekeeper.py
That way the tests and the code would live in the same root folder which might make it easier for contributors to manage.
For tests on the Helper Libraries themselves, the second "tests" feels redundant. Is there a standard for unit tests for Python? Maybe we should follow that (and for the other languages as well.
Anyway, obviously VSCode let me down when I created the folder. It'll be fixed shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of any standards for tests and I've seen it done many different ways. I think it would be best to keep the tests separate, since they don't really have a place in a normal installation. To do this, they'd need to go into a separate directory off of the root of the repo. Under that directory, they'd have the normal directory structure so that they can be copied over an existing installation, or patched in with a symlink.
There's a lot to sort out for testing and this is just a rough cut to get them stashed somewhere. It will be nice to have some tests in the repo to have some experience using them to help determine the best location and workflow for them. There were some other tests built up for core.metadata and core.date, and I have pages of regression tests that I use after major changes in OHC. We'll get there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, well the commit below moved the tests to the folder you suggested above. This issue should be addressed.
Community/Hysteresis/automation/lib/python/community/hysteresis.py
Outdated
Show resolved
Hide resolved
Community/Hysteresis/automation/lib/python/community/hysteresis.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Rich Koshak <[email protected]>
from org.openhab.core.library.types import QuantityType, DecimalType, PercentType | ||
except: | ||
from org.eclipse.smarthome.core.library.types import QuantityType, DecimalType, PercentType | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't needed. We already have core.jsr223.scope imported, and these are in there. So just use scope.QuantityType, etc.
One of these days, I'll change the import so that it's importing only what we need instead of the entire scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
All raised issued have been addressed. This PR is ready for review. |
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 [email protected]