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

Initial submittion of a reusable hysteresis function. #222

Closed
wants to merge 9 commits into from

Conversation

rkoshak
Copy link

@rkoshak rkoshak commented Aug 30, 2019

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

Signed-off-by: Rich Koshak <rlkoshak@gmail.com>
@rkoshak
Copy link
Author

rkoshak commented Aug 30, 2019

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

@CrazyIvan359
Copy link
Contributor

You beat me to it!

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

@rkoshak
Copy link
Author

rkoshak commented Aug 30, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

rkoshak commented Aug 30, 2019

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

@CrazyIvan359
Copy link
Contributor

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

@rkoshak
Copy link
Author

rkoshak commented Sep 3, 2019

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
Copy link
Contributor

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

@5iver
Copy link
Member

5iver commented Sep 3, 2019

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. 😄

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
Copy link
Author

rkoshak commented Sep 3, 2019

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. ;-)

Copy link
Member

@5iver 5iver left a 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.

Sphinx/Python/Community/Hysteresis.rst Outdated Show resolved Hide resolved

return rval

# Hysteresis test
Copy link
Member

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.

Copy link
Member

@5iver 5iver Sep 4, 2019

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!

Copy link
Author

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.

Copy link
Member

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.

Copy link
Author

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.

.github/Tests/core/utils.py Outdated Show resolved Hide resolved
Core/automation/lib/python/core/utils.py Outdated Show resolved Hide resolved
Signed-off-by: Rich Koshak <rlkoshak@gmail.com>
from org.openhab.core.library.types import QuantityType, DecimalType, PercentType
except:
from org.eclipse.smarthome.core.library.types import QuantityType, DecimalType, PercentType

Copy link
Member

@5iver 5iver Sep 5, 2019

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.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@rkoshak
Copy link
Author

rkoshak commented Nov 14, 2019

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

@rkoshak rkoshak closed this Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants