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

Improve variable naming consistency #2812

Open
2 tasks done
mark-epstein opened this issue Mar 20, 2024 · 3 comments
Open
2 tasks done

Improve variable naming consistency #2812

mark-epstein opened this issue Mar 20, 2024 · 3 comments
Labels
area/backend Something in the core of companion Enhancement New feature or request Idea Solution needed Needs a solution on how to solve it

Comments

@mark-epstein
Copy link

Is this a feature relevant to companion itself, and not a module?

  • I believe this to be a feature for companion, not a module

Is there an existing issue for this?

  • I have searched the existing issues

Describe the feature

Depending where you are in Companion a custom variable is referred to as
$(internal:custom_myVar) {variable substitutions}
A custom variable (internal:custom_myVar) {trigger events, conditions}
A custom variable (myVar) {trigger actions}

I would prefer that they were all a bit more consistent (and as brief as possible without introducing ambiguity. I'd suggest:
$(custom:myVar) for the substitution one and
A custom variable (myVar) for everywhere that's a drop-down option

The "internal" designation seems redundant in all cases because it appears that all custom variables, by default, are internal.

Usecases

just makes things a bit cleaner, easier to read

@dnmeid
Copy link
Member

dnmeid commented Mar 22, 2024

That's not going to happen like you suggest. There is a logic behind variable naming and the syntax of using variable names.
Every variable is issued by a module, that is the part in front of the :. The module internal offers some variables which are defined by the application and the user can add custom variables. To make them distinguishable custom variables start always with custom_. So a complete custom variable needs all the parts internal:custom_myVar
If you want to address a variable it depends on the context of where you are what you need to enter. E.g. in a text environment we need to distinguish between the literal text internal:custom_myVar and the call to that variable. This is done by surrounding variable names with $(). If we offer a dropdown, an action or an API call where only custom variable names are allowed we can shorten it to the last part as all custom variables are issued by internal and start with custom_
I admit that this sometimes is not intuitive at first and maybe there could be improvements in GUI and/or documentation to make variable selection more easy.
If you have any suggestions how we can improve here, please feel free.

I also admit that the naming syntax itself is not the best stuff.
In expressions you have to use variables with $() again just for consistency. Then there is the template string syntax with ${} which can be confusing with $(). As of today I would prefer being able to use variables in expressions without $() and also I would prefer the js-like notation of internal.custom.myVar. This would give modules a standardized way of grouping variables in a hierachical tree. Also if we would have variable replacement in expressions without $(), it would be good to replace the $()-notation in text with ${} notation. By doing so, we could make the standard text entry a template string and give it more functionality and more consistency with an expression at the same time.
Last but not least we could discuss if we move custom variables to top level. That means a custom variable could be as short as custom:myVar (or potentially custom.myVar). That would also mean that 'custom' would be a forbidden name for connections, like 'internal' already is.
But those are heavy changes. They don't have to be breaking as we can use our upgrades to automatically upgrade stuff and the APIs can support both syntax without conflicts. Anyhow, many users will need to learn some new patterns and I think we should change variable naming only if there is a broad support for this idea.

@dnmeid dnmeid added Enhancement New feature or request Solution needed Needs a solution on how to solve it Idea area/backend Something in the core of companion labels Mar 22, 2024
@mark-epstein
Copy link
Author

Thank you for the thorough explanation!

I'm curious, since the internal: indicates the module and custom_ distinguishes those variables from the stock ones, would it be of any benefit to have custom variables handled by their own module?

I didn't really need the variable names to change, I was just suggesting their presentation in the UI be simplified.

Regardless, as I mentioned in my original post, this request is little more than aesthetic and I doubt it would have the broad support you would want. Even I would put this near the bottom of a priority list, but until you explained why, I couldn't know how much an undertaking this was.

I do like the idea you suggest of js-like notation.

@Julusian
Copy link
Member

Julusian commented Apr 9, 2024

I have just realised that A custom variable is technically a 'user' owned description of a custom variable. But I never did expose a way for that to be edited by the user...

I'm not keen on changing A custom variable (internal:custom_myVar) to A custom variable (myVar), as this is supposed to be showing the name of the variable. And when the day comes that the description is editable, having that prefix will help reinforce that it is a custom variable, and not simply a special variable called $(myVar)

I've made the trigger actions dropdown match the A custom variable (internal:custom_myVar) so it is at least more uniform now.


That would also mean that 'custom' would be a forbidden name for connections, like 'internal' already is.

It turns out that custom already is a reserved word, since v3.0 when we introduced stricter rules on what characters can be used. I don't think connections were forced to have their labels fixed, as long as you don't try to edit their config, so it is possible that existing users have configs which will conflict with this.

// Check a few reserved words
if (label.toLowerCase() === 'internal' || label.toLowerCase() === 'companion' || label.toLowerCase() === 'custom')
return false

I'm curious, since the internal: indicates the module and custom_ distinguishes those variables from the stock ones, would it be of any benefit to have custom variables handled by their own module?

I have a feeling it was done under as internal:custom_ originally because of limitations/complexity in the architecture and flows. But after the reworking that was done for 3.0 and continued since then, I am confident that it wouldn't be too bad to do it under custom: and keep the old names as hidden aliases.


I also admit that the naming syntax itself is not the best stuff.

Yeah that is true. I did put a fair amount of effort into expanding what expressions can do, but didn't try to invent much new syntax, which has resulted in the ${$(aaa)} monstrosity.

I don't know if any of what you proposed has to be a breaking change, it doesn't sound like it would be too bad to retain support for the existing syntax too.

If we do go for a more js syntax, I would be tempted to do it as vars.module.someVarName instead (ie, under a global object), to reinforce that it is a variable, which will help ensure we don't have collisions.
I am specifically thinking here about clashes with some of the builtin functions, or once we allow declaring variables inside of an expression, such as:

a = 5
`${vars.internal.test} - ${a}`

although I'm not sure I like the lack of return or something there, which is something else to figure out when that is done

One challenge with that js syntax, is that module variables are allowed to contain . characters. Which might become problematic as I was hoping to start supporting a second colon in variable names as part of my pitch for 'properties to replace variables'. Not a dealbreaker, as that may not even happen, but it will make things a bit ambiguous in both the expression, and when fetching the value of a variable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend Something in the core of companion Enhancement New feature or request Idea Solution needed Needs a solution on how to solve it
Projects
None yet
Development

No branches or pull requests

3 participants