Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add JythonScriptEngineFactory #7208

Closed
wants to merge 5 commits into from

Conversation

5iver
Copy link

@5iver 5iver commented Mar 23, 2020

This pull request adds functionality for a JythonScriptEngineFactory and replaces openhab/openhab-core#1253. This PR only includes Jython 2.7.2 without any helper libraries, which have been submitted as a separate addon (#7210).

Signed-off-by: Scott Rushworth openhab@5iver.com

5iver pushed a commit to 5iver/openhab2-addons that referenced this pull request Mar 23, 2020
openhab#7208 (comment).

Signed-off-by: Scott Rushworth <openhab@5iver.com>
@cpmeister cpmeister added the enhancement An enhancement or new feature for an existing add-on label Mar 23, 2020
@5iver 5iver force-pushed the Add_JythonScriptEngineFactory branch 3 times, most recently from 41c49d1 to 383be8c Compare March 25, 2020 07:06
5iver pushed a commit to 5iver/openhab2-addons that referenced this pull request Mar 25, 2020
This will should be merged after I have created a separate package for
the helper libraries, as discussed here...
openhab#7208 (comment).
For now, this was built to test using the helper libraries as a
separate bundle.

Signed-off-by: Scott Rushworth <openhab@5iver.com>
5iver pushed a commit to 5iver/openhab2-addons that referenced this pull request Mar 25, 2020
This will should be merged after I have created a separate package for
the helper libraries, as discussed here...
openhab#7208 (comment).
For now, this was built to test using the helper libraries as a
separate bundle.

Signed-off-by: Scott Rushworth <openhab@5iver.com>
5iver pushed a commit to 5iver/openhab2-addons that referenced this pull request Mar 25, 2020
This will should be merged after I have created a separate package for
the helper libraries, as discussed here...
openhab#7208 (comment).
For now, this was built to test using the helper libraries as a
separate bundle.

Signed-off-by: Scott Rushworth <openhab@5iver.com>
5iver pushed a commit to 5iver/openhab2-addons that referenced this pull request Mar 25, 2020
This will should be merged after I have created a separate package for
the helper libraries, as discussed here...
openhab#7208 (comment).
For now, this was built to test using the helper libraries as a
separate bundle.

Signed-off-by: Scott Rushworth <openhab@5iver.com>
@5iver 5iver force-pushed the Add_JythonScriptEngineFactory branch 3 times, most recently from 6028e49 to 6cdb895 Compare March 25, 2020 09:30
5iver pushed a commit to 5iver/openhab2-addons that referenced this pull request Mar 25, 2020
This will should be merged after I have created a separate package for
the helper libraries, as discussed here...
openhab#7208 (comment).
For now, this was built to test using the helper libraries as a
separate bundle.

Signed-off-by: Scott Rushworth <openhab@5iver.com>
5iver pushed a commit to 5iver/openhab2-addons that referenced this pull request Mar 25, 2020
This will should be merged after I have created a separate package for
the helper libraries, as discussed here...
openhab#7208 (comment).
For now, this was built to test using the helper libraries as a
separate bundle.

Signed-off-by: Scott Rushworth <openhab@5iver.com>
@5iver 5iver force-pushed the Add_JythonScriptEngineFactory branch 3 times, most recently from 5db9d8f to 61ab8b9 Compare March 25, 2020 22:02
5iver pushed a commit to 5iver/openhab2-addons that referenced this pull request Mar 25, 2020
This will should be merged after I have created a separate package for
the helper libraries, as discussed here...
openhab#7208 (comment).
For now, this was built to test using the helper libraries as a
separate bundle.

Signed-off-by: Scott Rushworth <openhab@5iver.com>
@5iver 5iver force-pushed the Add_JythonScriptEngineFactory branch from d98ea61 to cc01e06 Compare March 31, 2020 16:31
5iver pushed a commit to 5iver/openhab2-addons that referenced this pull request Mar 31, 2020
This will should be merged after I have created a separate package for
the helper libraries, as discussed here...
openhab#7208 (comment).
For now, this was built to test using the helper libraries as a
separate bundle.

Signed-off-by: Scott Rushworth <openhab@5iver.com>
@5iver 5iver force-pushed the Add_JythonScriptEngineFactory branch from ab745e2 to a6e4b54 Compare March 31, 2020 18:45
@5iver 5iver force-pushed the Add_JythonScriptEngineFactory branch from a6e4b54 to a4f9a91 Compare April 20, 2020 07:42
@5iver 5iver requested a review from a team as a code owner April 20, 2020 07:42
@5iver 5iver force-pushed the Add_JythonScriptEngineFactory branch from a4f9a91 to 6951449 Compare April 20, 2020 13:00
@openhab openhab deleted a comment from TravisBuddy Apr 20, 2020
@openhab openhab deleted a comment from TravisBuddy Apr 20, 2020
@openhab openhab deleted a comment from TravisBuddy Apr 20, 2020
@openhab openhab deleted a comment from TravisBuddy Apr 20, 2020
Scott Rushworth added 3 commits July 2, 2020 06:33
Signed-off-by: Scott Rushworth <openhab@5iver.com>
Signed-off-by: Scott Rushworth <openhab@5iver.com>
Signed-off-by: Scott Rushworth <openhab@5iver.com>
@5iver 5iver force-pushed the Add_JythonScriptEngineFactory branch from 7db1aa8 to 625cb00 Compare July 2, 2020 13:09
@5iver
Copy link
Author

5iver commented Jul 2, 2020

@cweitkamp, thank you again for the review! It is very nice to see this building properly! I think I have made all of your suggested changes, except for removing the bnd file, which I believe is necessary.

Signed-off-by: Scott Rushworth <openhab@5iver.com>
@TravisBuddy
Copy link

Travis tests were successful

Hey @openhab-5iver,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@openhab openhab deleted a comment from TravisBuddy Jul 2, 2020
@openhab openhab deleted a comment from TravisBuddy Jul 2, 2020
Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

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

Alright. I tested it and it is working. From my side an approval.

We now have to reach an agreement on the namespace in openhab/openhab-core#1319.

@5iver
Copy link
Author

5iver commented Jul 2, 2020

Yes, there is the question of the namespace, and also whether the helper libraries need to be included with the ScriptEngineFactories. I've provided as much of an argument as I can muster for the namespace that I used here (I have several more SEFs in queue!) and why it would be best to keep helper libraries separated from the SEFs. It would be very much appreciated if you could weigh in on these in 1319!

Thanks again for the review! If you are interested in an addon for the Jython helper libraries, #7210 also needs reviewing 🙂.

@5iver 5iver requested a review from kaikreuzer July 2, 2020 16:01
@kaikreuzer
Copy link
Member

Yes, there is the question of the namespace

Honestly, there is not really a question on this. Add-ons in this repo have to follow the naming convention org.openhab.<addontype>.<id>.

My suggestion for this PR was hence org.openhab.automation.jythonscripting, but I'd be fine with any other speaking id as well.

@5iver
Copy link
Author

5iver commented Jul 18, 2020

There are many more ScriptEngineFactory add-ons to submit, possibly some more helper library add-ons, but I would much rather work on a scripting API than helper libraries for each scripting language, and there are many community helper libraries that could be made into add-ons. Putting them all into this namespace will be ugly and difficult to manage, as I pointed out in openhab/openhab-core#1319 (comment). However, I will accept whatever namespace you force me to use in order to move this PR forward and to finally get it merged into 2.5.x. I suggest then to just take out the separators from the proposal I made three months ago...

org.openhab.automation.scriptenginefactoryjython
org.openhab.automation.helperlibrariesjython
org.openhab.automation.helperlibrariesjythonareatriggersandactions

@kaikreuzer
Copy link
Member

I have the feeling that we talk about very different things and thus do not find to a common understanding here - see my comment on openhab/openhab-core#1319 (comment).

@TravisBuddy
Copy link

Travis tests have failed

Hey @openhab-5iver,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@openhab-bot
Copy link
Collaborator

Automatic code migration to openHAB 3 succeeded.

The resulting code can be found at https://ci.openhab.org/job/openHAB-Addons-Migration/79/artifact/bundles/.

You can download the migrated code from there and create a new PR against the master branch of the openhab-addons repo to contribute it for being included in openHAB 3.x.

Please see this issue about the details on how to proceed with your existing PR.

@5iver 5iver closed this Oct 9, 2020
@5iver 5iver deleted the Add_JythonScriptEngineFactory branch October 9, 2020 20:11
@kaikreuzer
Copy link
Member

@openhab-5iver Did you port this PR against the main branch? If you require any help, please let me know!

@wborn wborn added new automation and removed enhancement An enhancement or new feature for an existing add-on labels Oct 17, 2020
@wborn wborn mentioned this pull request Dec 16, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants