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

SCRIPT transformation syntax and file naming #3465

Closed
jimtng opened this issue Mar 18, 2023 · 11 comments · Fixed by #3487
Closed

SCRIPT transformation syntax and file naming #3465

jimtng opened this issue Mar 18, 2023 · 11 comments · Fixed by #3487
Labels
enhancement An enhancement or new feature of the Core

Comments

@jimtng
Copy link
Contributor

jimtng commented Mar 18, 2023

I wish that the SCRIPT transformation's syntax uses the underlying scripting engine's extension, i.e. just .js, .rb, etc. instead of .script. I understand that the reason it's .script now is because it's the name of the transformation itself (i.e. SCRIPT transformation -> .script file extension, MAP transformation -> .map file extension). As a result of this choice, an extra field must be supplied to indicate the script type (rb, graaljs, dsl, etc).

But it would be so much nicer to have SCRIPT(myscript.rb) and SCRIPT(myscript.js) rather than SCRIPT(rb:myscript.script) and SCRIPT(graaljs:myscript.script):

  • It looks nicer. The scriptType is deduced from the file extension just like the scripts in conf/automation
  • The syntax is simpler and more straight forward. One less field. If necessary, this first field can be provided to avoid ambiguity e.g. graaljs vs nashorn, if that's still relevant. This way it can be backwards compatible with the current syntax. When the first field is provided, it overrides the scriptType, but if it's absent, deduce the scriptType from the given file extension
  • We get the correct syntax highlighting and editor behaviour while editing myscript.rb instead of myscript.script because the editor recognises the file extension
  • When looking through the files inside OPENHAB_CONF/transform, when we see myscript.rb and myscript.js, we'd know exactly which language they're written in, instead of myscript.script
@J-N-K
Copy link
Member

J-N-K commented Mar 18, 2023

I can't find the discussion immediately but this has been discussed before and we agreed that files in the transformation folder should have an extension that is their transformation type. My vote would be "won't fix".

@wborn wborn added the enhancement An enhancement or new feature of the Core label Mar 18, 2023
@wborn
Copy link
Member

wborn commented Mar 19, 2023

A link to the previous discussion would be appreciated!

If necessary, this first field can be provided to avoid ambiguity e.g. graaljs vs nashorn, if that's still relevant.

With jsr223 rules files the .graaljs and .nashornjs extensions can already be used for avoiding ambiguity. Usually you can customize your OS and editors to add additional extensions for opening editors and syntax highlighting.

@J-N-K
Copy link
Member

J-N-K commented Mar 19, 2023

I‘ll see if I can find it. Removing the language: prefix would bring us into trouble with the inline scripts where we don‘t have an extension to derive the language from.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 19, 2023

No, don't remove it completely. Make it optional, both for compatibility and for the cases when it's needed. So SCRIPT(graaljs:myscript.script) SCRIPT(graaljs:myscript.js) and SCRIPT(myscript.js) are all supported.

In the case where SCRIPT(my script.script) is used, or when myscript.js alone is not sufficient because there are two js engines available,then raise an error/warning

@J-N-K
Copy link
Member

J-N-K commented Mar 19, 2023

Another case where it would be needed: UI based transformations. I almost forgot about them because openhab/openhab-webui#1448 is now approaching its first birthday, but there we also need the prefix.

It might be personal choice, but I think that a consistent way to configure the same thing is better than different ways which only work under special conditions.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 19, 2023

Another case where it would be needed: UI based transformations. I almost forgot about them because openhab/openhab-webui#1448 is now approaching its first birthday, but there we also need the prefix.

It might be personal choice, but I think that a consistent way to configure the same thing is better than different ways which only work under special conditions.

Even the webui editor needs a hint of what type of script a .script file is. Using an actual .js extension would remove that need. That too seems like a huge win for the usability factor. The nashorn/graaljs is a temporary bridging issue going forward there is no ambiguity, as nashorn is going to be dropped, right?

The transformation editor would need to become a generic script editor loading whatever language depending on the extension in the same way the rule/script editor is.

You never specified the transformation with only the filename. You always have to specify the transformation type e.g. SCRIPT(), MAP(), JSONPATH(), REGEX(), etc. So the type of transformation is already represented by them. The rest is the extra data for the corresponding transformation service to be interpreted individually by the respective service.

It seems to be only natural to say SCRIPT(something.js). In the case of inline script SCRIPT(graaljs:|......) which is why we still support the first field containing the scripttype.

@J-N-K
Copy link
Member

J-N-K commented Mar 19, 2023

From the transformation POV we have a UID and we request that UID from the transformation registry. We don't care about the UID itself or its structure. The UID can be a filename (
name_<language>.<type>) if the transformation is loaded from the transform folder or something like config:<type>:<name>:<language> if provided by the ManagedTransformationProvider (i.e. UI). <name> is not restricted in any way (except allowed characters), you can't determine the scripting language from that.

So we would introduce a new special solution for file-based configuration which additionally is totally anti-pattern. I don't think that specifying the language as prefix is a big burden for the user.

@J-N-K
Copy link
Member

J-N-K commented Mar 19, 2023

I also found the discussion, it's in #2872 and especially #2872 (comment) lead to the removal of the .js and .dsl support I originally implemented.

@ccutrer
Copy link
Contributor

ccutrer commented Mar 22, 2023

I want to also chime in that forcing a .script extension on script transformations is counterintuitive - not just to humans, but also other software. The easiest example is editors that use the file extension for syntax validation and highlighting. But also, the Ruby addon supports writing unit tests for (Ruby) script transformations, but in order to do so I'm forced to write extra comments at the beginning of the file to identify it as Ruby, as opposed to any JS script transformations I have that also have a .script extension.

Usually you can customize your OS and editors to add additional extensions for opening editors and syntax highlighting.

This only works if you only use one language for script transformations. Again, if someone has script transformations in multiple languages (say I write Ruby, but found a community contribution in JavaScript, I'd have both).

Maybe I can come at this by re-stating from a slightly different point of view. The current reasoning is "extensions of files in the transformations folder inform which transformation provider should be used," no? Traditionally, transformation providers only support a single syntax, so that has been 1:1. But the script transformation provider is a polyglot, or a proxy to other languages. So saying ".rb is handled by SCRIPT provider, as well as .js is handled by SCRIPT provider" doesn't violate my original re-statement.

It just seems like with JS transformation gone in 4.0 and forcing people to use SCRIPT, we'd want to make it as simple as possible for them - just replace JS( with SCRIPT(, and be on your way. Infer the type from the extension if possible, still allow explicit script types, and error if script type can't be inferred, same as if the file can't be found. This doesn't seem like all that much of a maintenance burden or anti-feature given what it provides. No other transformation requires an extra argument to help with this, so it can be confusing when you first start using script transformations. I know I was definitely confused by it when I wrote my first one.

@J-N-K
Copy link
Member

J-N-K commented Mar 22, 2023

Again: There is not only file-based transformation but also UI based/managed transformations. There is no way to determine the scripting language from an arbitrary UID. Why should we restrict the UID to a "special" format (with characters not allows in UIDs! and different from all other transformations) to allow something for file-based transformations that is different from all other transformations?

The concept "file extension=script type" is super easy to understand, well-known to existing users and IMO not counterintuitive in any way.

@ccutrer
Copy link
Contributor

ccutrer commented Mar 22, 2023

There is not only file-based transformation but also UI based/managed transformations.

Again: just because there are UI based/managed transformations, does not mean you can't make it a good experience for file-based transformations. Especially if it doesn't harm UI based/managed transformations.

Why should we restrict the UID to a "special" format

I don't recall anyone advocating restricting UIDs. Simply for file based transformations, if it has an extension, and not an explicit script type, infer the script type from the extension.

"file extension=script type"

Except it is very counter-intuitive, because you just made an ambiguous presumption there. I know you meant "script type" to mean "the type of this transformation is script", but skimming it it is easy to read "the file extension indicates the type of script it is. like ruby".

Keep in mind, you wrote the feature. So everything about it would be automatically intuitive to you. We're trying to point out that for other people (especially someone who has never used a transformation before, or only used the JS transformation in the past) it is not intuitive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants