Proposal: make variable expansion optional

10 views
Skip to first unread message

Ingo Albrecht

unread,
Oct 10, 2020, 10:52:20 AM10/10/20
to klis...@googlegroups.com
Hello!

I have mentioned before that I am working on a python plugin. That
plugin uses high-level language constructs to access clish state and
does therefore not require variable expansion.

Keeping expansion enabled would be acceptable for most cases, but it
would be cleaner to disable it in cases such as these. This patch is a
proposal on how to do that.

Details on how it works are in the patch.

Greetings
prom
0001-Made-variable-expansion-optional.patch

Serj Kalichev

unread,
Oct 12, 2020, 5:21:15 AM10/12/20
to klis...@googlegroups.com
Hello!

It's interesting patch. We use Lua for ACTIONs and it can be usefull for
us to disable expansion. But I need a time to think about implementation.

The problem is klish will have three "objects" that control expansion.
1. symbol
2. ACTION (higher priority)
3. STARTUP

Klish has several options that have defaults (in STARTUP) and higher
priority value in ACTION. I think it will be good to set default value
for "expand" for all XMLs in STARTUP. So it's not obvious what
priorities is good for this case. ACTION has higher priority than
STARTUP but priorities of ACTION and symbols are not so clean.

Is it good both symbol and ACTION have values for "expand"?

Is it good idea to set default "expand" to false (patch sets false for
ACTION)? Probably it can break compatibility.

So it's a subject for discussion.


10.10.2020 17:52, Ingo Albrecht пишет:

Serj Kalichev

unread,
Oct 12, 2020, 5:39:28 AM10/12/20
to klis...@googlegroups.com
May be STARTUP's "expand" is really not necessary in this case.

There is one bad thing in idea of symbol's "expand". XML writer knows
nothing about expansion without special plugin (symbol) documentation.



10.10.2020 17:52, Ingo Albrecht пишет:

Serj Kalichev

unread,
Oct 12, 2020, 10:19:55 AM10/12/20
to klis...@googlegroups.com
We have discussed the feature with coworkers who actively use klish.
It's a good feature.
Now it's a simple in implementation but a bit complex for understanding.
For example suppose we have symbol with enabled "expand" but then write
"<ACTION expand="false">". The expected behaviour the expansion is
disabled (ACTION can override symbol properties) but really it's
enabled. Because expand="false" really means "get property from symbols".

We propose another scheme:

There are free sources of "expat" settings:
1. <ACTION expand="true/false">. The highest priority. Three states
(true/false/not_defined). The "true" enables expanding, the "false"
disables it, "not_defined" (not defined in XML) leads to using settings
from level with less priority.
2. Symbol's property. Less priority level. Three states
(true/false/not_defined). Not defined state leads to using settings from
low priority level.
3. <STARUP default_expand=...>. Lowest priority. Two states
(true/false). True by default.

So symbol can override global default. ACTION can override symbols.

I think it's most compatible and flexible variant.
What do you think about it?



10.10.2020 17:52, Ingo Albrecht пишет:

Ingo Albrecht

unread,
Oct 12, 2020, 11:34:50 PM10/12/20
to klis...@googlegroups.com
On 10/12/20 4:19 PM, Serj Kalichev wrote:
> We have discussed the feature with coworkers who actively use klish.
> It's a good feature.
> Now it's a simple in implementation but a bit complex for understanding.
> For example suppose we have symbol with enabled "expand" but then write
> "<ACTION expand="false">". The expected behaviour the expansion is
> disabled (ACTION can override symbol properties) but really it's
> enabled. Because expand="false" really means "get property from symbols".

Yes, that is true. I wanted to go for "simple first". In my current
implementation the semantics are more like "expand_required".

> We propose another scheme:
>
> There are free sources of "expat" settings:
> 1. <ACTION expand="true/false">. The highest priority. Three states
> (true/false/not_defined). The "true" enables expanding, the "false"
> disables it, "not_defined" (not defined in XML) leads to using settings
> from level with less priority.
> 2. Symbol's property. Less priority level. Three states
> (true/false/not_defined). Not defined state leads to using settings from
> low priority level.
> 3. <STARUP default_expand=...>. Lowest priority. Two states
> (true/false). True by default.
>
> So symbol can override global default. ACTION can override symbols.
>
> I think it's most compatible and flexible variant.
> What do you think about it?

Willing to implement this because it does fully cover my needs.

Going to need an enum type to represent those tri-state values because
these go in instance fields - in contrast to all other tri-state
handling which is contained in "clish/shell/shell_xml.c".

Do you have a preference on how to represent this? If not then I will
just propose something in a few days.

Many Thanks!
prom

=========================================================================
Remark on PLUGIN

There is one further case that we might think about: the contents of the
PLUGIN element. Currently nothing gets expanded there, not even vars.
Proposing that we leave this like it is for now. One could argue that
this is really a hidden ACTION, although it gets treated differently. It
has no locking, no logging, no interrupt flag and so on. This is okay
for some plugins, but for the interpreters one might want these things
at some point.

=========================================================================
Remark on STARTUP

Personally I am not so much a fan of STARTUP and the "global magic
module" that it implies, but with a sane default it's not a problem.

I think that this could bear some further discussion, but it would get
off topic.

Let me just mention that I have patches that eliminate the need for
STARTUP in an attempt to improve module composition possibilities.

=========================================================================

Ingo Albrecht

unread,
Oct 13, 2020, 1:37:20 AM10/13/20
to klis...@googlegroups.com


On 10/12/20 11:39 AM, Serj Kalichev wrote:
> May be STARTUP's "expand" is really not necessary in this case.
>
> There is one bad thing in idea of symbol's "expand". XML writer knows
> nothing about expansion without special plugin (symbol) documentation.

That is true, but one could argue that the XML writer has to know the
plugin anyway and won't be surprised. Using expansion itself also
potentially requires knowledge of magic things like expansion modifiers.

All-in-all I am happy with the solution that you have proposed because
it is backwards-compatible, minimal and covers my need for defaulting
expansion to "off" on interpreter symbols.

There is always room for improvement. Let's do simple first.

Thanks
prom


What follows is some further discussion just for thought.


### Safety perspective

One can argue that expansion is inherently unsafe because of escaping
issues and should therefore only be used when strictly necessary.

From this perspective the plugin is actually protecting the XML writer
from a dangerous construct by disabling expansion. One might even want
to disable expansion by default for this reason.

There is, however, the shell action and the fact that clish uses
expansion for configuration actions. Enabling expansion explicitly on
all of these might be a solution.

Configuration actions could migrate away from using expansion, but that
is another topic. One could argue that most XML writers never need to
use these directly since they are all special-purpose (history, help,
plugin debugging). This is not 100% true though.


### Related discussion about SQLite plugin

[caution: experimental thinking]

In my experimental SQLite plugin I have secondary actions (open/close
database) for which I use params as arguments and provide no script at all.

This does work okay for pre-defined commands in plugin-supplied XML
modules, but it it not discoverable for XML writers.

One parameter that is particularly important is the one that indicates
which database to use. An alternative would be to use a late-bound
dynamic sym named "${db}@sqlite", which I currently prefer as a
solution. This does not solve the issue of other parameters though.
SQLite might also be able to deal with this natively. Commented inline
syntax could also be used.

There also is the issue of not having multiple actions. Entering a view
might require opening several databases, and that would in turn require
multiple actions unless implemented using some sort of "ini" style
format that can also indicate options for multiple databases (read-only,
sync, sharing, locking).

A completely different avenue would be extending the XML on a per-plugin
basis using XML namespaces and using that for defining database
bindings. All that would be required of clish in this case would be to
ignore out-of-namespace XML. Not sure if it does, but it should anyway.

Serj Kalichev

unread,
Oct 13, 2020, 9:08:39 AM10/13/20
to klis...@googlegroups.com
I will write tri-state implementation. I want to include it to "faux"
library and then port to old "lub" library.

I suppose to use body of PLUGIN tag as something like INI-file. The lub
library has engine to easy parse INI-files.
I don't understand a text about PLUGIN. You write that no vars expanded
within PLUGIN. Why is it "hidden ACTION"? Probably it's mean "no ACTION
at all".

What do you use instead STARTUP tag to specify starting view? Is it
command line?



13.10.2020 06:34, Ingo Albrecht пишет:

Serj Kalichev

unread,
Oct 13, 2020, 10:14:34 AM10/13/20
to klis...@googlegroups.com
See tri_t type from lub/types.h


13.10.2020 06:34, Ingo Albrecht пишет:

Ingo Albrecht

unread,
Oct 14, 2020, 5:18:30 AM10/14/20
to klis...@googlegroups.com
On 10/13/20 4:14 PM, Serj Kalichev wrote:
> See tri_t type from lub/types.h
>

Cool. Will send reworked patch.
Reply all
Reply to author
Forward
0 new messages