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.

Sign up for GitHub

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

Jump to bottom

[jythonscripting] Refactor, improve and simplify #16508

Merged
merged 4 commits into from Mar 17, 2024
Merged

[jythonscripting] Refactor, improve and simplify #16508

merged 4 commits into from Mar 17, 2024

Conversation

HolgerHees
Copy link
Contributor

@HolgerHees HolgerHees commented Mar 10, 2024 •
edited

I refactored the Jython binding a bit

  • I moved all classes to a 'internal' subpackage like other scripting addons
  • I refactored JythonScriptEngineFactory with the following objectives
    • I change the way how script types are detected, to a stream api based variant like in groovy
    • Also the way how to PyScriptEngineFactory is instantiated is changed (No more looping over all script engines)
    • I simplified and reorganized the way of python path handling a bit, to make it easier to read
  • I implemented a ScriptFileWatcher to deal with path like in other scripting addons
    • scripts can be placed in /automation/jython
    • libs can be placed in /automation/jython/lib.
    • "jython" is used, because "python" should be reserved for a graalvm based implemention (I'm already working on a prototype)
  • I removed an additional deprecated info
  • I added some missing parts in addon.xml

additionally I tested this refactored jython addon with success in my openhab 4.1.1 production environment.

@HolgerHees HolgerHees requested a review from a team as a code owner March 10, 2024 10:36
@HolgerHees
Copy link
Contributor Author

This pull request replaces the previous one, which I closed due to a mistake

#16503

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/jython-is-not-deprecated/154187/23

@HolgerHees HolgerHees changed the title [jython][WIP] Cleanups and simplifications [jython][WIP] Refactoring, improvement and simplifications Mar 10, 2024
@HolgerHees HolgerHees changed the title [jython][WIP] Refactoring, improvement and simplifications [jython][WIP] Refactoring, improvement and simplification Mar 10, 2024
@HolgerHees HolgerHees changed the title [jython][WIP] Refactoring, improvement and simplification [jython] Refactoring, improvement and simplification Mar 14, 2024
@jlaur jlaur added the enhancement An enhancement or new feature for an existing add-on label Mar 16, 2024
@jlaur jlaur changed the title [jython] Refactoring, improvement and simplification [jython] Refactor, improve and simplify Mar 16, 2024
@jlaur jlaur changed the title [jython] Refactor, improve and simplify [jythonscripting] Refactor, improve and simplify Mar 16, 2024
@jlaur jlaur mentioned this pull request Mar 16, 2024
Merged
wborn
wborn reviewed Mar 17, 2024
View reviewed changes
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

Thanks for improving the Jython add-on! If you plan on taking care of it maybe also update the CODEOWNERS file? 🙂

...src/main/java/org/openhab/automation/jythonscripting/internal/JythonScriptEngineFactory.java Outdated Show resolved Hide resolved
@HolgerHees
Copy link
Contributor Author

HolgerHees commented Mar 17, 2024 •
edited

@wborn can you give me a tip as a newbie, whats wrong with the CI/Build pipeline. I was already running it locally, with the same result, but without more understanding from my side. The only thing I know is that it is related to a dependency openhab-runtime-base => openhab-core-model-persistence .... org.eclipse.xtext.common.types

but I have not changed anything which could touch this

also this issues #7798 was not helpful

maybe something is broken in openhab-core

@jlaur
Copy link
Contributor

jlaur commented Mar 17, 2024

@wborn can you give me a tip as a newbie, whats wrong with the CI/Build pipeline.

See https://community.openhab.org/t/failed-to-execute-goal-org-apache-karaf-tooling4-4-4-verify/154678/4

@jlaur jlaur added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Mar 17, 2024
@HolgerHees
Copy link
Contributor Author

HolgerHees commented Mar 17, 2024 •
edited

@jlaur thanks for cleaning review requests, It was happen because I tried a rebase with openhab:main instead of a merge.

and my latest forced push was reverting it.

  • UPDATE *

my rebase was creating a new commit, because of my previous merge with openhab:master. I reverted this merge too and now my rebase was working properly.

HolgerHees and others added 4 commits March 17, 2024 21:12
@HolgerHees
- moved implementation to 'internal'
4c605fb 
- refactored JythonScriptEngineFactory
- implemented JythonScriptFileWatcher
- fixed addon.xml & package-info.java

Signed-off-by: Holger Hees <holger.hees@gmail.com>
@HolgerHees @wborn
Simplify stream list collector in JythonScriptEngineFactory
be98c4b 
Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: Holger Hees <holger.hees@gmail.com>
@HolgerHees
changed codeowner for jythonscripting
0448abe 
Signed-off-by: Holger Hees <holger.hees@gmail.com>
@HolgerHees
organized imports in JythonScriptEngineFactory
00c28b3 
Signed-off-by: Holger Hees <holger.hees@gmail.com>
wborn
wborn approved these changes Mar 17, 2024
View reviewed changes
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

Thank you!

@wborn wborn merged commit e857f32 into openhab:main Mar 17, 2024
3 checks passed
@wborn wborn added this to the 4.2 milestone Mar 17, 2024
magx2 pushed a commit to magx2/openhab2-addons that referenced this pull request Mar 24, 2024
@HolgerHees @magx2
[jythonscripting] Refactor, improve and simplify (openhab#16508)
0f0b29c 
* moved implementation to 'internal'
* refactored JythonScriptEngineFactory
* implemented JythonScriptFileWatcher
* fixed addon.xml & package-info.java
* simplify stream list collector in JythonScriptEngineFactory
* changed codeowner for jythonscripting
* organized imports in JythonScriptEngineFactory

Signed-off-by: Holger Hees <holger.hees@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlaur jlaur jlaur left review comments

@wborn wborn wborn approved these changes

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@HolgerHees @openhab-bot @jlaur @wborn