libubox: Add anonymous strings, ints, et al in arrays

[pprindeville]

You can have a "name" in an array member without an associated value.

[pprindeville]

@Ansuel Can someone please review this?

[pprindeville]

cc: @robimarko @neheb @hauke @ynezz

[pprindeville]

cc: @nbd168

[pprindeville]

@i-jonas I thank you for taking the time to diligently review this.

[l-jonas]

It looks like json_push_str and json_push_int are not necessary at all. I found the documentation at https://openwrt.org/docs/guide-developer/jshn.

. /usr/share/libubox/jshn.sh
json_init
json_add_array test
json_add_int "" 1
json_add_int "" 2
json_add_int "" 3
json_add_string "" "eins"
json_add_string "" "zwei"
json_add_string "" "drei"
json_close_array
json_dump

just results in { "test": [ 1, 2, 3, "eins", "zwei", "drei" ] }

[pprindeville]

It looks like json_push_str and json_push_int are not necessary at all. I found the documentation at https://openwrt.org/docs/guide-developer/jshn.

. /usr/share/libubox/jshn.sh
json_init
json_add_array test
json_add_int "" 1
json_add_int "" 2
json_add_int "" 3
json_add_string "" "eins"
json_add_string "" "zwei"
json_add_string "" "drei"
json_close_array
json_dump

just results in { "test": [ 1, 2, 3, "eins", "zwei", "drei" ] }

And that's usually the point of convenience functions. They provide a simplified wrapper for doing something else.

As I point out elsewhere, though, there are packages like netifd which provide their own version of the same convenience function: I'd rather collect all of these in one place rather than reinventing the wheel. There's no cost to having extra convenience functions... it's interpreted code so no object bloat. And it makes the code easier to read.

[l-jonas]

They provide a simplified wrapper fro doing something else.

Why are they not implemented as wrappers around the existing functions in the public API like your referenced code examples do it?

Why are the new warnings not implemented in a way that code not using the new convenience functions also benefits from it?

their own version of the same convenience function

Most of your referenced source code parts are domain specific convenience functions that cannot be compared with that. Those that are wrappers are implemented better than your suggestion, e.g. by allowing to add multiple array elements at once. Why does your convenience function not support that?

it's interpreted code so no object bloat

Instead, its source code bloat. I don't see a difference here.

And it makes the code easier to read.

For me, it does not make a difference.

[pprindeville]

They provide a simplified wrapper fro doing something else.

Why are they not implemented as wrappers around the existing functions in the public API like your referenced code examples do it?

No idea. Perhaps they also gave up on making changes to libubox?

Why are the new warnings not implemented in a way that code not using the new convenience functions also benefits from it?

Sorry, how do you mean? How would that work?

their own version of the same convenience function

Most of your referenced source code parts are domain specific convenience functions that cannot be compared with that. Those that are wrappers are implemented better than your suggestion, e.g. by allowing to add multiple array elements at once. Why does your convenience function not support that?

Disagree. My proposed json_push_string does what _proto_push_string but with checking to make sure it's happening within an array:

https://github.com/openwrt/netifd/blob/master/scripts/netifd-proto.sh#L240-L242

it's interpreted code so no object bloat

Instead, its source code bloat. I don't see a difference here.

Then what... we stop adding any new code at all?

And it makes the code easier to read.

For me, it does not make a difference.

How much are you having to write scripting with jshn? I've had to jump into it and it's more of a lift than it should be.

[l-jonas]

No idea. Perhaps they also gave up on making changes to libubox?

I was saying that the examples you referenced implemented it better than you from my perspective. Now I was asking why you implement it differently?

Sorry, how do you mean? How would that work?

Take a look at that other implementations. Then think about the call stack.

Disagree.

Disagree with the disagree. The check can be added at another place where it affects all calls, including those using the current public API. See previous point.

e.g. by allowing to add multiple array elements at once

Please check the links you provided again. This feels to me like an AI research result that throws links without understanding their content.

Then what... we stop adding any new code at all?

No. I just dislike the argument "no bloat because not compiled".

I've had to jump into it and it's more of a lift than it should be.

Using another function name to skip one empty argument? This particular thing is not that helpful. I've worked with JSON in OpenWrt in shell scripts and really liked replacing that with ucode.

[pprindeville]

No idea. Perhaps they also gave up on making changes to libubox?

I was saying that the examples you referenced implemented it better than you from my perspective. Now I was asking why you implement it differently?

All my examples are calling json_add_string "" arg... is that what we're talking about? And you're saying that's better than json_push_string arg as I'm proposing?

Sorry, how do you mean? How would that work?

Take a look at that other implementations. Then think about the call stack.

I've lost the thread. Which other implementations? Can you post a link?

Disagree.

Disagree with the disagree. The check can be added at another place where it affects all calls, including those using the current public API. See previous point.

But... that was the point of adding a wrapper called json_push_string that explicitly tests for being inside an array... which simply chaining to json_add_string "" "$1" wouldn't do.

e.g. by allowing to add multiple array elements at once

Please check the links you provided again. This feels to me like an AI research result that throws links without understanding their content.

Are we talking about the links mentioned here?

#12 (comment)

I grepped through the sources and manually copy & pasted those links.

Then what... we stop adding any new code at all?

No. I just dislike the argument "no bloat because not compiled".

Any new functionality comes with some cost unless it includes serious refactoring to shrink overall size. Even there the cost is risk of introducing new bugs or regressions.

I've had to jump into it and it's more of a lift than it should be.

Using another function name to skip one empty argument? This particular thing is not that helpful. I've worked with JSON in OpenWrt in shell scripts and really liked replacing that with ucode.

Perhaps but I like having commonality between ISC-DHCP and Kea. Makes it easier for me.

[l-jonas]

All my examples are calling json_add_string "" arg... is that what we're talking about? And you're saying that's better than json_push_string arg as I'm proposing?

Yes

But... that was the point of adding a wrapper called json_push_string that explicitly tests for being inside an array... which simply chaining to json_add_string "" "$1" wouldn't do.

json_add_string could do some check depending on the first argument empty or not. So you could warn for keys in an array and no keys in an object. Or maybe _json_add_generic to make it independent from string or integer.

I grepped through the sources and manually copy & pasted those links.

I believe that and still made some experiences with junior devs. That's why it feels like that. I was referring to https://github.com/openwrt/openwrt/blob/main/package/system/procd/files/procd.sh#L114 which adds some value over a simple wrapper.

Any new functionality comes with some cost unless it includes serious refactoring to shrink overall size. Even there the cost is risk of introducing new bugs or regressions.

I know. And due to this, one has to balance the advantages and the risks.

Perhaps but I like having commonality between ISC-DHCP and Kea. Makes it easier for me.

How is this related to one isolated script converting the configuration?

[pprindeville]

Perhaps but I like having commonality between ISC-DHCP and Kea. Makes it easier for me.

How is this related to one isolated script converting the configuration?

It shares a lot of code with dhcpd.init: the validation of the UCI, the synthesis of the resources records and integration with Bind is all the same. All that's different is the generation of the DHCP configuration target file itself.

[pprindeville]

Separating out json_get_index in case that alone is palatable. See PR #33.

[l-jonas]

It shares a lot of code with dhcpd.init

Sharing by duplicating? If there would be a third package like dhcp-common containing the shared code, then I would call and consider it sharing.

[pprindeville]

It shares a lot of code with dhcpd.init

Sharing by duplicating? If there would be a third package like dhcp-common containing the shared code, then I would call and consider it sharing.

But they'll never both be installed at the same time.

[l-jonas]

But they'll never both be installed at the same time.

I'm thinking about the source code duplication. You could try to use a symlink within this repository to actually share code between to packages, but the common package sounds cleaner.

[pprindeville]

Added test coverage and complete scalars

cc: @nbd168 @jow- @systemcrash @Ansuel

[pprindeville]

cc: @Noltari

[pprindeville]

Anyone?

[robimarko]

Thanks! Rebased on top of master and merged!