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

Support a cleaner syntax for SCRIPT transformation #3476

Closed
wants to merge 2 commits into from

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Mar 23, 2023

Support SCRIPT(file.rb) syntax as an alternative, not a replacement, to SCRIPT(rb:file.script)
Also SCRIPT(file.rb?param1=x) works like SCRIPT(rb:file.script?param1=x)

The original syntax is still fully supported, so this is not a breaking change.

This makes changing from JS transformation to SCRIPT transformation as easy as changing JS(myscript.js) to SCRIPT(myscript.js)

Discussed in #3465

I understand that @J-N-K disagreed with this proposal. However, I feel very strongly about making the script transformation syntax more elegant and intuitive for file-based scripts and I have created this PR as a PoC to demonstrate what I wish to achieve.

Unrelated to the matter, some extra error checks were added to the regex pattern to avoid errors, e.g. when an inline transformation script had a ? character as part of the code, it would've been extracted as parameters, which shouldn't be the case.

If this doesn't negatively affect anything else, I'd appreciate further considerations for this PR. If there are any potential issues that I'm not aware of, I'll try to address them also.

…file.script)`

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng jimtng requested a review from a team as a code owner March 23, 2023 06:21
@J-N-K
Copy link
Member

J-N-K commented Mar 23, 2023

This breaks the ConfigOptionProvider and some scripts will show up when creating profiles (.script and managed) and some will not (.js, .rb, ...). This is far more confusing than prefixing the script language.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 23, 2023

This breaks the ConfigOptionProvider and some scripts will show up when creating profiles (.script and managed) and some will not (.js, .rb, ...). This is far more confusing than prefixing the script language.

Thanks for the feedback. I believe I can fix that.

…stry

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng
Copy link
Contributor Author

jimtng commented Mar 23, 2023

This breaks the ConfigOptionProvider and some scripts will show up when creating profiles (.script and managed) and some will not (.js, .rb, ...)

I've added some extra code to include all the supported scriptTypes loaded in the transformation registry.

If we are going to infer the scriptengine / language from the extension here too, we can remove the support for .script completely, although it will be a breaking change, but it is one that actually improves usability as it simplifies the UI by reducing errors. Currently, the user can select the wrong language especially in the UI when all they see was myscript.script they don't have a hint as to the real language inside that file. If we removed the .script and only support the actual languages' extensions, this wouldn't happen.

In the case of managed provider, the language is defined when creating the transformation, and the user should not need to choose/decide again when loading/selecting the transformation.

I am merely mentioning this as an observation. It is not the goal / purpose / scope of this PR to make such a drastic change, but I'd be happy to expand the scope of this PR if deemed necessary.

image

@J-N-K
Copy link
Member

J-N-K commented Mar 23, 2023

I don't understand where you see the "extra burden", selecting the language is a common task when configuring openHAB. If you add a rule with a script action, you have to select the language, too. As I already said: I believe this is wrong and complicates things all the way for the benefit of a few.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 23, 2023

I don't understand where you see the "extra burden", selecting the language is a common task when configuring openHAB.

It seems quite clear to me:

  • You decide which language to use when creating the .script or managed transformation
  • You are given a number of *.script files and managed transformations as an option
  • Then again you have to select which language to execute that file/managed transformation on this screen and each and every time you need to select a transformation.
  • User is allowed to make the wrong language choice here. At the very least, if you've created those transformation a long time ago, you'd have to look into the file again to check which language it is before making the choice here. It is a completely unnecessary step. Why make things harder? To me this is a huge burden that cannot be overstated. Try creating 20 links with 20 transformations, and on each and every link you have to choose the language + the script. Wouldn't you prefer only choosing one thing vs two things?

versus:

  • You decide which language to use, at the creation of the script, whether it is a file, or a managed transformation.
  • You are given a number of *.js, *.rb, *.groovy, etc, + managed transformations, all of which already carry their own association to the corresponding language, so the user doesn't have to choose the language again when selecting the transformation

It greatly simplifies the UI and eliminates one possible error.

To me .script is the complicated way.

However, as I said, I'm not here to advocate for changing the use of .script in this PR. People can continue to use .script exclusively and have no trace of per-language extensions showing in their system should they choose to do so.

I believe this is wrong and complicates things all the way for the benefit of a few.

Indeed this is a matter of opinion and I'm at the opposite end of that opinion. As I understand it, openHAB is a community project, so is there a democratic process in making a decision or is it a case of the decision is made only by certain individuals? Should we create a poll or something to see whether this only benefits "a few" or the "majority"?

Again, I'm quite happy to leave .script as is if that's your preferred way of working with the transformation, as long as .js / .rb etc is also supported.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 23, 2023

If you add a rule with a script action, you have to select the language, too.

Yes, but only at the time of creation of that rule. Not when you are merely selecting from a list of existing rules such as demonstrated in the following screenshots

image

Here you're given a list of existing rules. You do not need to specify in which language the rule should be executed, because indeed that is irrelevant here, and you should not be allowed to execute a JS rule with anything other than a JS engine.
image

@J-N-K
Copy link
Member

J-N-K commented Mar 23, 2023

Do what you want, I won't veto that. But I'm pretty sure that this will end in a full nightmare for @rkoshak when support is needed.

@ccutrer
Copy link
Contributor

ccutrer commented Mar 23, 2023

Do what you want, I won't veto that. But I'm pretty sure that this will end in a full nightmare for @rkoshak when support is needed.

You keep saying that inferring the script type is somehow more complicated than having to specify it, without giving any actual examples of what is more complicated. Up to this point I thought you meant that it would make the implementation and interfaces complicated, but this comment makes me think maybe you have something in mind that not having to specify the script language will somehow be more complicated for the user to use. Do you have a specific example in mind where you think a user might be confused?

@J-N-K
Copy link
Member

J-N-K commented Mar 23, 2023

Every time there are several ways to do the same thing this ends up with people mixing instructions from different places. The result is that they start complaining something is "not working".

I have said everything I have to say in this issue. My opinion is that it's a mistake to give up simple design principles (which have proven to be easy to understand for a long time) to add additional ways for doing what is already possible with what we have.

@ccutrer
Copy link
Contributor

ccutrer commented Mar 23, 2023

Wow, that argument basically reads like "we shouldn't make simpler ways to do things, because sometimes people will get confused." Why was Main UI (and Paper UI before it) even written if you could already administer openHAB completely via files?!

@jimtng even posited that we could entirely get rid of having to choose the scripting language at all (when referencing transformations, not when writing new ones). That would mean there are no additional ways to do the same thing (though there would be a small period of additional support burden if there needs to be manual steps when updating openHAB, but the transformation could still undocumented support the old way), not adding to the support burden.

I still haven't seen an argument I understand for not making the user repetitively specify the scripting language violates "simple design principles".

@J-N-K
Copy link
Member

J-N-K commented Mar 23, 2023

Because it's much harder to mess things up in UI configuration than in files. Look at the forum: 2/3 of the issues are related to file based configuration. It should be removed completely, but that is probably not something I can get a majority of maintainers to support.

What I'm saying is that "map transformation uses .map", "scale transformation uses .scale" and "script transformation uses .script" is much easier than "map transformation uses .map", "scale transformation uses .scale" and "script transformation uses .script or whatever scripting language you want". Except when you configure via UI, then you have to use script.

If you want to go that route, do it fully: remove .script, extend the TransformationService interface with a method .getSupportedTransformations, implement that in all transformation add-ons and refactor TransformationResource to use that method so that UI and files-based transformations behave the same way.

@ccutrer
Copy link
Contributor

ccutrer commented Mar 23, 2023

[file based configuration] should be removed completely

Got it. You believe file based configuration should be removed completely, so anything that improves the file-based experience isn't worth doing. I'm not being facetious or sarcastic here. It helps me to understand from the point of view your arguments come.

What I'm saying is that "map transformation uses .map", "scale transformation uses .scale" and "script transformation uses .script" is much easier than "map transformation uses .map", "scale transformation uses .scale" and "script transformation uses .script or whatever scripting language you want".

I think our argument is that people don't ask "what extension does a script transformation use? obviously it's .script! now I'm going throw some javascript in there", they say "I have a javascript transformation. that's implicitly a .js file. now which transformation do I use for this? oh, script, okay!"

Except when you configure via UI, then you have to use script.

Why are you thinking about the file extension at all when creating a transformation in the UI??

If you want to go that route, do it fully: remove .script

Yes, @jimtng suggested that, since it would also simplify the UI way.

@J-N-K
Copy link
Member

J-N-K commented Mar 23, 2023

[file based configuration] should be removed completely

Got it. You believe file based configuration should be removed completely, so anything that improves the file-based experience isn't worth doing.

I didn‘t say that and this really marks the end of the discussion for me.

@rkoshak
Copy link

rkoshak commented Mar 23, 2023

I've watched this thread and the other one.

From my perspective, the .script file extension has always been unsatisfactory for all the reasons mentioned here already and a few more reasons. Yes, using the .script extension is consistent with the other transformations but it's inconsistent with rule file extensions which each have their own file extensions even given that in addition they have separate folders to further separate and distinguish them. There's also overlap with Rules DSL Scripts. So it's consistent with the other transformations but in OH writ large it poses it has inconsistencies.

Also, when users create a SCRIPT transformation, I'm not sure they are thinking about it as a SCRIPT transform so much as a jRuby transform or a Rules DSL transform. I know that's the mental model I fall into when I think about it. Whether or not that is reasonable I can't say, but it's what I do. @J-N-K, I'm sure you think about it as one thing given you wrote it that way and have the most familiarity with it. The SCRIPT part isn't as important to the end user as the language the transform is written in because it's the language that is going to dictate the syntax. The SCRIPT transformation itself, mentally, is like the engine that the scripts run in.

It also really seems odd and redundant, in the profile case, to have to choose the language. It seems like the language the transform is written in should be somehow inferred from the script itself. Why does the user need to choose the language when creating the script and then have to remember and choose it again when applying it? (Not to mention that screen seems to indicate that you can't have the incoming and outgoing transform script for a profile written in different languages). But even if we kept it where the user selects the language, wouldn't it be nice if upon selecting the language only those scripts written in that language were shown to select from.

So I am sympathetic to the arguments made by @jimtng and @ccutrer. However, I don't really know what the scripts will look like when defined in the UI. I don't understand the implications of what's being proposed and how that is different (from the end user's perspective) so I can't really weigh in on the counter argument without more of an idea how that will look. As far as I can tell, at least on build #3383, we don't have a UI for creating transformation scripts yet. But from what I can gather, the filename is part of the scripts' UID and therefore it: 1. must conform to the UID restrictions for allowable characters (meaning we can't have Myfile.script.js for example) and we want to make sure the ID follows the same pattern whether the transform is defined by file or through the UI.

It's already somewhat of a problem that applying it on a label follows SCRIPT(js:mytransform.script):%s for labels, SCRIPT(js:mytransform.script) for file based Profiles (I'm guessing), transform('SCRIPT', 'js:mytransform.script, someVariable)in a rule (Rules DSL example) andSCRIPT:js:mytransform.script` for at least the HTTP and MQTT bindings (maybe more?). Doubling these by having different IDs for file based transforms and UI based transforms would indeed be a nightmare.

At a high level:

  • yes, it should look and mostly work the same no matter how the scripts are defined; we should not have one type of ID for file based and another for UI based.
  • I'd personally like to see the feature somehow infer or know what the language the transform is written in so the user doesn't have to select it twice, once when creating the script and again when applying it (file extension? 🤷, value in the transformation script JSONDB?, something else? ) or, if the user does select it twice once selected the second time when choosing it it only offers those .scripts that are written in that language.

That's my perspective, for what it's worth.

@ccutrer
Copy link
Contributor

ccutrer commented Mar 23, 2023

I didn‘t say that and this really marks the end of the discussion for me.

:(. I hope I didn't offend you. My personality is that I'm a direct and frank person, but I'm honestly not trying to be offensive, insulting, sarcastic, etc.

Now I'm brainstorming here:

WHAT IF?

What if the script transformation, instead of being a proper transformation service itself was simply a meta-transformation service that listens for new instances of script engines to be registered, and when they are it dynamically registers a new transformation service instance specifically for that engine? Then we could properly say "the JS transformation" or the "Ruby transformation." They would be referenced as "JS(mytransform.js)" from file-based configuration, which has the double-effect of making the proper extension match the transformation's name, which seems to be very important to @J-N-K, as well as restoring some semblance of backwards compatibility with openHAB 3.4's JS transform (though you'd have to install the JS automation addon, instead of the JS transformation provider; honestly I see this as a good change, because then it's up to each addon to document its support of transformations, as opposed to the SCRIPT transformation which has poor discoverability in the docs because it's not actually an addon at all - it's part of core). The UI would automatically change to simply choosing "JS transformation" instead of "script transformation" and then "JS". Other transformation providers wouldn't need any change to satisfy any (now unnecessary) interface changes to support a single service supporting multiple file types. There are probably other pitfalls to this approach that I'm unaware of, and would be happy to have them pointed out to me.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 24, 2023

Do what you want, I won't veto that.

If you want to go that route, do it fully: remove .script, extend the TransformationService interface with a method .getSupportedTransformations, implement that in all transformation add-ons and refactor TransformationResource to use that method so that UI and files-based transformations behave the same way.

Thanks for the pointers. I think that would be the ideal scenario, which would address all the issues raised here.

@J-N-K
Copy link
Member

J-N-K commented Mar 24, 2023

What if the script transformation, instead of being a proper transformation service itself was simply a meta-transformation service that listens for new instances of script engines to be registered, and when they are it dynamically registers a new transformation service instance specifically for that engine? Then we could properly say "the JS transformation" or the "Ruby transformation." They would be referenced as "JS(mytransform.js)" from file-based

This won't happen. We would be in a situation even worse than before when we had only "JS Transformation": "Why can I use JS, DSL, Groovy and Ruby but not Python?". The transformation is universal and this will not change. A required metadata for scripting addons contradicts that.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 24, 2023

@rkoshak if we were to change the SCRIPT transformation syntax to script.js syntax, do you think we should:
a. completely drop the js:file.script syntax in 4.0, or
b. silently support it so as not to break people's existing installation, but not mention it in the documentation,

While (b) sounds nice, eventually we'll have to drop it anyway (when?) and we'll simply delay the inevitable. You know that if we maintain support, people will not go out of their way to change it until it breaks.

@rkoshak
Copy link

rkoshak commented Mar 24, 2023

@rkoshak if we were to change the SCRIPT transformation syntax to script.js syntax, do you think we should:
a. completely drop the js:file.script syntax in 4.0, or
b. silently support it so as not to break people's existing installation, but not mention it in the documentation,

I don't really have a position either way really largely because I don't know whether/how either choice impacts the UI created script transformations.

If we did "b", I would not leave it out of the docs. If it's supported, it should be documented, even if it's marked as deprecated in the docs. I really don't like undocumented features.

If we do a, we should make sure it's done before OH 4.0 release. It's a breaking change for sure.

@ccutrer
Copy link
Contributor

ccutrer commented Mar 24, 2023

What if the script transformation, instead of being a proper transformation service itself was simply a meta-transformation service that listens for new instances of script engines to be registered, and when they are it dynamically registers a new transformation service instance specifically for that engine? Then we could properly say "the JS transformation" or the "Ruby transformation." They would be referenced as "JS(mytransform.js)" from file-based

This won't happen. We would be in a situation even worse than before when we had only "JS Transformation": "Why can I use JS, DSL, Groovy and Ruby but not Python?". The transformation is universal and this will not change. A required metadata for scripting addons contradicts that.

I didn't say any required metadata. I said meta-transformation service. One in core, that's not actually a concrete transformation service, but would enumerate script engines as they're registered, and dynamically create a new transformation service for that script engine. So Python would automatically get one, even though it's not maintained.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 24, 2023

UI created script transformations.

How do you create a UI script transformation? AFAIK it isn't implemented yet.

If/when it's implemented, it should contain information about the scriptType / engine that it uses. The scriptType in SCRIPT(scriptType:scriptUID) syntax is needed only for identifying scriptType because of the chosen generic extension in "filename.script" currently. That would be fixed by simply allowing the actual language's extension filename.js / filename.rb etc.

UI transformation being created in the UI will be retrieved through its scriptUID as a Transformation object and which contains scriptType information. It can be made to return scriptType not as script (which currently pertains to the name of the transformation) but as its actual script language, and then they will be checked against the currently loaded script engines, in exactly the same manner as file-based transformation scripts will be.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 24, 2023

The UI would automatically change to simply choosing "JS transformation" instead of "script transformation" and then "JS". Other transformation providers wouldn't need any change to satisfy any (now unnecessary) interface changes to support a single service supporting multiple file types. There are probably other pitfalls to this approach that I'm unaware of, and would be happy to have them pointed out to me.

meta-transformation service. One in core, that's not actually a concrete transformation service, but would enumerate script engines as they're registered, and dynamically create a new transformation service for that script engine. So Python would automatically get one, even though it's not maintained.

This sounds very cool, although the implementation would be a bit "more complex" than the currently "static" SCRIPT transformation.

Thinking out loud here: once implemented, the SCRIPT() transformation itself would cease to exist as far as the user is concerned, and we would simply refer to the "script transformation" as being done using the language's identifier JS(), RB(), GROOVY(), JAVA(), DSL(). Then in the UI, if you've got one or more of the scripting language addons installed, it would list them as separate types of transformation and when selected, it will list only transformations written in that selected language.

I think practically, from the end user's point of view, this is the smoothest and easiest to understand transition from the old JS transformation.

image

@rkoshak
Copy link

rkoshak commented Mar 25, 2023

How do you create a UI script transformation? AFAIK it isn't implemented yet.

There is an open PR created for it to add a transformations registry and the other stuff required in core. I'm not sure the status of it beyond a lot of work has been put into it. Once that's done there will need to be changes to MainUI to support it too which I don't think exists.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 25, 2023

There is an open PR created for it to add a transformations registry and the other stuff required in core. I'm not sure the status of it beyond a lot of work has been put into it.

Ah yes, I found that PR. I believe it can be adapted.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 26, 2023

superseded by #3487

@jimtng jimtng closed this Mar 26, 2023
@jimtng jimtng deleted the transformation-script-ext branch August 21, 2023 23:05
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.

None yet

4 participants