-
-
Notifications
You must be signed in to change notification settings - Fork 708
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
JSR223 documentation #428
JSR223 documentation #428
Conversation
Hey Steve 👋 🎉 great to see you are making progress! First comment. It seems your commit was not created by a registered GitHub user mail address. You've also forgotten to sign-off-by your commit.
I never saw that warning :) thanks ;) I'll look at the content itself soon! |
I've already got one very general question. I hope to not incur somebody's hatred 😅 Is "JSR223 Javascript Scripting" the name we want to use in the long run? Yes that it is the technically correct term but it's far from something the regular user can easily refer to. Now would be the right time to think of and establish a "marketing term" for this new functionality (OH2). Everyone, wdyt? |
I think I've amended the commit properly, but I'm not a git expert so let me know if I haven't done it correctly. I also set up the recommended git commit hook for future commits.
I'm open to any naming. That phrase is only used in the page browser title and not in the page content (AFAIK). Do have some suggestions for alternative names? |
I removed the questions from documentation text and listed them below:
From my perspective, after the open JSR223 script UID bug is fixed the documentation is good enough for an initial release. |
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 really like this, I agree with @steve-bate and say that it'd be good to merge as is. There are some lint issues such as:
-
should be used instead of*
for defining lists.- Sentences in paragraphs should start on an immediate new line.
I don't know enough about the style-guidelines yet to comment more on this though.
I had a few very minor comments.
configuration/jsr223.md
Outdated
{% include base.html %} | ||
|
||
# JSR223 Scripting | ||
> Note: This feature is for advanced users who have or are willing to learn some programming skills and are comfortable working with the command line prompt of the operating system hosting openHAB. |
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.
Honestly, I'd remove the "advanced" word here to avoid putting people off initially. If they're willing to learn, they'll get there eventually.
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.
Changed.
configuration/jsr223.md
Outdated
|
||
* [Oracle Nashorn](http://www.oracle.com/technetwork/articles/java/jf14-nashorn-2126515.html) (Javascript bundled with Java) | ||
* [Jython](http://www.jython.org/) | ||
* [Apache Groovy](http://www.groovy-lang.org/) |
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.
Could you add a summary phrase to Jython and Apache Groovy? Although fairly obvious, people may want to know that Jython is specifically python like.
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 was thinking that people not familiar with the languages would use the links to get more information.
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'd agree with @BClark09, there should be just a few words for orientation
configuration/jsr223.md
Outdated
script2.py | ||
``` | ||
|
||
the load order will be: `00script.py`, `01/script1.py`, `script2.py`. |
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.
Can you add another file to this example, such as 01/scriptx.py
? It's not obvious in which order this is expected to run in the case where the file in a directory is alphabetically after a file in the top directory.
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've added the example. I think the current file loading is very confusing and should be improved. Fortunately, most users will just have scripts in the top-level directory. I need to study the source and do some experimentation to be 100% confident that my current description is fully accurate.
configuration/jsr223.md
Outdated
---------|-------------| | ||
`State` | `org.eclipse.smarthome.core.types.State` | | ||
`Command` | `org.eclipse.smarthome.core.types.State` | | ||
`DateTime` | `org.joda.time.DateTime` (if Jodatime is available) | |
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.
General interest question, isn't joda.time always available?
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.
There is conditional logic in the source code for the available of the Joda library. I don't know if this was because of potential OSGI classloader issues for some other reason. Maybe @smerschjohann can comment.
configuration/jsr223-jython.md
Outdated
|
||
Look for any potentially related error messages. | ||
|
||
To enable debug logging, use the [Karaf logging]({{base}}/administration/logging.html) commands to enabled debug logging for the automation functionality: |
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.
enabled -> enable
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.
Changed.
configuration/jsr223-jython.md
Outdated
LoggerFactory.getLogger("org.eclipse.smarthome.automation.examples").info("Hello world!") | ||
``` | ||
|
||
The openHAB server uses the [SLFJ](https://www.slf4j.org/) library for logging. Jython can [import Java classes](http://www.jython.org/jythonb Depending on the openHAB logging configuration, you may need to prefix logger names with `org.eclipse.smarthome.automation` for them to show up in the log file (or you modify the logging configuration). |
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.
The line seems like it should belong before the example above it.
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.
Moved.
configuration/jsr223-jython.md
Outdated
... [INFO ] [.smarthome.automation.examples:-2 ] - Hello world! | ||
``` | ||
|
||
Notice that no rules were required for this simple script. This can be a useful way to experiment with simple Jython code before defining rules or other mroe advanced functionality. |
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.
mroe -> more
Rather, that sentence would read better as: "This can be a useful way to experiment with simple Jython code before defining advanced rules and functionality."
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 think creating rules are the primary and relatively simple use case for JSR223. When I write "advanced functionality", I'm referring to defining other openHAB components (rule modules, things, console command, ...) or using JSR223 to integrate external libraries like Esper CEP, Apache Camel.
configuration/jsr223-jython.md
Outdated
|
||
## Script Examples | ||
|
||
Jython scripts provide access to almost all the functionality in an openHAB runtime environment. As a simple example, the following script logs "Hello, World!" into the openhab log file. Note that `print` will usually not work since the output has no terminal to display the text. One exception is when the openHAB server is running from the Eclipse IDE. |
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.
openhab log file. -> openHAB log file.
OR
openhab log file. -> openhab.log file.
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.
Changed.
configuration/jsr223.md
Outdated
|
||
## Overview | ||
|
||
[JSR223](https://docs.oracle.com/javase/6/docs/technotes/guides/scripting/) ([spec](https://jcp.org/aboutJava/communityprocess/pr/jsr223/index.html)) is a standard scripting API for Java Virtual Machine (JVM) [languages](https://en.wikipedia.org/wiki/List_of_JVM_languages). The JVM languages provide varying levels of support for the JSR223 API and interoperability with the Java runtime. Currently the following languages are know to work well for openHAB scripting. |
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.
are know -> are known
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.
Changed.
Thanks! I appreciate the thorough review. I've made changes to address most of the issues you've raised.
I changed the bullets, but I wasn't sure I understood the second item. Are you saying that every sentence should start a new line? Just curious... what is the rational for that and the use of '-' instead of '*' (since both are valid Markdown syntax)?
Obviously, I don't know the guidelines either. I tried searching Google and the openHAB docs domain-specific search and couldn't find a reference. If a guideline exists, we should make it easier to find. |
@steve-bate the reason for that is simple. The style guide is pretty new and I'm writing a style guide article and am updating all existing articles accordingly just these last couple of days 😃 See here for all details: #423 The most important part of the "style guide project" is the remark-lint configuration already provided with the PR and ready to be used. You can execute
Every sentence should be on a new line, correct. Markdown will still concatenate sentences but short lines play better with the line-based git versioning. See first paragraph here
See PR :) |
I added a short description to the scripting language list. I also fixed most of the remark-lint issues. Do we think we'll be able to merge this initial version soon? A few users in the community forums have been asking about it. |
Signed-off-by: Steve Bate <[email protected]> (github: steve-bate)
Squashed the commits. |
All changes look good to me! |
The article looks great! Let's merge so users can already benefit from it. @steve-bate the two sub-pages do not own a menu entry. In my opinion that should never happen. Wdyt? Would you mind adding them as submenu entries for the existing one? @steve-bate @BClark09 In one of my first comments I asked if we should go with "JSR223 Scripting" in the long run? I feel the name might be too technical to attract the broad audience, maybe a simpler catchy name would be better? Do you think it's worth discussing that further? |
I think Java 9 includes this functionality natively and if were're going to migrate to that then this I guess (a wild one at that) support might have to change considerably, so definitely not in the loooong run. The problem is being able to serve those who know one of the scripting languages supported by JSR223, and those that know it includes more than that.
|
I like the idea of moving away from using "JSR223" name to describe this scripting support. I quickly looked at the Java 9 scripting API and it looks very similar to JSR223 (same package, class names, etc.) so wouldn't expect the functionality to change much. In the future we may regret using the |
I've moved the discussion. @steve-bate did you read my comment regarding the menu? |
Oh sorry, I did miss that. I must have pass over it when I was reading the thread on my phone.
Do you mean the menu in the left side bar? Or a different menu? |
The one on the left, yes (this one) |
Initial documentation for using the JSR223 support in openHAB2. I expect that the documentation will be extended and improved over time.
There are several "TODO" comments that refer to open questions. I'm hoping @smerschjohann can provide clarification on some of those. Thanks.
I also slightly changed the Jekyll config to remove the deprecation warnings about the
gems
keyword.@ThomDietrich @kaikreuzer