multiple "--set <pref>" options might behave inconsistent if one entry is invalid

[juergenRe]

Evaluation of preferences settings will behave inconsistent:

• --set mqtt.enabled 1 --set 0 1 will write nothing at all and show an error hint on the last
• --set 0 1 --set mqtt.enabled 1 will write the correct setting but show no error
• --set mqtt.enabled 1 --set mqtt 1 --set mqtt.username abc will write the 2 valid entries but again show no error.

Reason is the way how preferences are updated within the callback onConnected() in the clause if args.set:. Evaluation handles the incoming list of arguments correctly, but does not take in account, that there might be a mix of correct and incorrect entries.

Before preparing a fix, I want to quickly check what is the intended behavior for such cases:
a) shall we discard all changes and just show the faulty entries?
b) shall we accept the correct entries, send them to the radio but also show all of which have failed?

What is your opinion?

[roberthadow]

@juergenRe If I were the designer I would choose your option (a). All or nothing.

What is my logic? A user may have deveoped a set of preferences, the new configuration including dependencies. Partial fulfillment of those preference changes would not achieve the goal. Particularly for a node to which the physical access is limited, partial execution would disable the unit. Here is an example:

meshtastic --tcp 192.168.1.200 \
    --set power.is_power_saving true \
    --set security.admin_key "base64:PASTEPUBLICKEYHERE" \
    --timeout 60

The changes don't lodge until the implied --reboot at the end of the meshatastic command.

If the first line were to execute, but not the second, one would be left with no way to control the node, save climbing and plugging in a USB.

In any case, ALL errors should be returned. My preference is that all specific preferences are echoed on success. "Writing modified channels to device" doesn't tell me much if I fat-fingered something.

In your example, it appears that the order of --set preference PREFERENCE makes a difference in results. That's bad. Nowhere in the documentation is described any importantce to order. That's a function of the architecture.

[juergenRe]

@roberthadow: Thanks for your input and explanation. Because I am very new to the project I prefer to get first some feedback before changing the code.
In any case, your description already gives me some valuable hints. Thanks again.

[roberthadow]

@juergenRe In all my wandering around this github repository, I have not seen a programming standards document. My background is in large systems development, but in the last century. Forgive me then, if I don't speak the language of Agile, Scrum and sprint.

More important than new features are the existing ones. Features and their names should be self-explanatory, consistent, meticulously documented, and recover gracefully when they fail. Here are a few observations about this project:

(1) Set and get methods should always be delivered in pairs. They should have a common form --set preference
PREFERENCE. The absolutely most important preferences, shortName and longName, deviate. There is no get method. Their form is --set-preference PREFERENCE.

(2) `For reasons unknown to me, snake_case and camelCase are used interchangeably, even for the same preference.

(3) Here's a preference someone forgot to add to the documentation: --set network.ipv4Config.ip PREFERENCE

I know I want an ipV4 address 192.168.1.200. --set network.ipv4Config.ip "192.168.1.200" fails every time. One needs an integer value. Don't use the common ip-to-integer conversion on the web. You need a special coversion for an ipV4 conversion of a.b.c.d.

d * 256^3 + c * 256^2 + b * 256^1 + a * 256^0

[juergenRe]

Robert, you are right about your observations - but I can't tell you about the reasons, as I mentioned I am new here. For the syntactical level, pylint will ensure a certain consistence. But you are right about the naming of setters/getters...

To avoid your comments just get buried within this issue, I propose you open your own issues in this repo. Then I can concentrate on a single function/correction here.

[juergenRe]

Fixed in PR #873