StringConverter.cpp/GetArgvBeginEnd: Bug or feature?

Moderators: grovkillen, Stuntteam, TD-er

Post Reply
Message
Author
Kim_Panda
Normal user
Posts: 41
Joined: 25 Jan 2021, 17:03

StringConverter.cpp/GetArgvBeginEnd: Bug or feature?

#1 Post by Kim_Panda » 02 Apr 2021, 20:10

while implementing the new command feature with a TaskNumber-prefix
https://espeasy.readthedocs.io/en/lates ... mmand.html ("Command a specific task for multiple instances of a plugin")
I found that multiple optional parameters seem not be supported properly and I wonder if that is bug or feature :?: :o
The problem: If you have two optional parameters like "command,a,b" you get in the pluginfrom the function "parseString(...)":

Code: Select all

par1 = parseString(string, 2); par2 = parseString(string, 3);

  command-string   par1    par2
--------------------------------------
1) command,a,b        a       b 
2) command,a          a         
3) command,,b         b      		<=== !!!
4) command,b          b      
I had instead expected for 3)
!) command,,b                 b 
and also the following delivers the same as 3)
5) command,,,,,b      b      
6) command, ,b        b      

The problem is with 3), where the second parameter b is returned as if it was entered as first parameter
and it is not possible to distinguish between 3) and 4) and also not 5).

With this implementation, it is in practice solely possible to have the very last parameter(s) optional, because multiple sequential separators are always treated as one.

The question is, if the current behaviour is a bug or a feature :?:

The problem, I think, is in the following sequence of "bool GetArgvBeginEnd(...)" in "StringConverter.cpp"

Code: Select all

    if       (!parenthesis && (c == ' ') && (d == ' ')) {}
    else if  (!parenthesis && (c == ' ') && (d == separator)) {}
    else if  (!parenthesis && (c == separator) && (d == ' ')) {}
    else if  (!parenthesis && (c == ' ') && (d >= 33) && (d <= 126)) {}   
    else if  (!parenthesis && (c == separator) && (d >= 33) && (d <= 126)) {}
    else
    {
The solution is probably in the 5th row, because the separator "," is treated as regular character between 33 and 126.
I had expected there, in the if-clause, a "&& (d != separator)" or similar, which might solve the issue.

User avatar
Ath
Normal user
Posts: 3415
Joined: 10 Jun 2018, 12:06
Location: NL

Re: StringConverter.cpp/GetArgvBeginEnd: Bug or feature?

#2 Post by Ath » 02 Apr 2021, 21:11

Kim_Panda wrote: 02 Apr 2021, 20:10 The solution is probably in the 5th row, because the separator "," is treated as regular character between 33 and 126.
I had expected there, in the if-clause, a "&& (d != separator)" or similar, which might solve the issue.
You're very welcome to create a separate PR for improving that behavior :D
/Ton (PayPal.me)

TD-er
Core team member
Posts: 8643
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: StringConverter.cpp/GetArgvBeginEnd: Bug or feature?

#3 Post by TD-er » 03 Apr 2021, 09:40

The current implementation Assumes value to be present or not. There is no "empty value".
It is now more like C++ function parameters. You either have something of the required type, or you have nothing.
This also enforces you only can have optional parameters at the end.

I don't see why current rules would break if we support empty variables.
The only thing that may cause issues is when people try to use empty variables on existing commands. What is the default value for existing commands?

Kim_Panda
Normal user
Posts: 41
Joined: 25 Jan 2021, 17:03

Re: StringConverter.cpp/GetArgvBeginEnd: Bug or feature?

#4 Post by Kim_Panda » 03 Apr 2021, 21:57

I don't fully understand, what you are going to say. But step by step:
TD-er wrote: 03 Apr 2021, 09:40 This also enforces you only can have optional parameters at the end.
Is this a strongly intended restriction/limitation? :?: It inhibits further flexibility.
TD-er wrote: 03 Apr 2021, 09:40 I don't see why current rules would break if we support empty variables.
The only thing that may cause issues is when people try to use empty variables on existing commands.
Do you mean, that you would take the risk to change this central function and you see only the risk, that existing code could assume, that e.g. if the 3rd parameter is provided, then the 2nd is also existing in any case?
Example: "command,a,,c" currently delivers par1="a", par2="c", par3=""
With the suggested change it would deliver par1="a", par2="", par3="c".
And you reckon, there is software that checks if par3 is non-empty and assumes that par2 is also non-empty, without a check for that fact. Hmmm. Is it that what you mean?
Well, that would cause a different behaviour and would be a risk. But is that kind of implementation likely?
TD-er wrote: 03 Apr 2021, 09:40 What is the default value for existing commands?
The value for a non existing parameter is "", what is currently already delivered by
GetArgv(), parseStringToEndKeepCase(), parseStringKeepCase(), parseString(), parseStringToEndKeepCase()

And honestly, why should a caller send a command like "command,a,,b" or even "command,a,,,,,,,,,b", when he wants to deliver par1 and par2 like in "command,a,b"? :roll:

OK, please tell me if you think its worth to go for the suggested change.
If yes, I would go for a try and I will do some limited testing, but I definitely can't do a complete regression test of the whole ESPEasy. That's where I count on your experience with ESPEasy to assess, if the risk is too high.
BTW, for my P003 enhancement, I did implement it now with only one optional parameter and abstained from the flexibility of having two optional parameters. So from my point, there is currently no need for that enhancement.
Please advice.

TD-er
Core team member
Posts: 8643
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: StringConverter.cpp/GetArgvBeginEnd: Bug or feature?

#5 Post by TD-er » 03 Apr 2021, 22:36

With the suggested change it would deliver par1="a", par2="", par3="c".
This is what I assumed you meant and how it should be.
Problem is that if you support "empty" parameters, people may use it.
So all existing commands should have a default value (or know the 'current' state) for all parameters.
And honestly, why should a caller send a command like "command,a,,b" or even "command,a,,,,,,,,,b", when he wants to deliver par1 and par2 like in "command,a,b"? :roll:
You should not.
For example what if you have a command to write something on a display.
e.g. writeToDisplay,column,row,text
What will be done when giving this command:

Code: Select all

writeToDisplay,,,"This is a text"
Is it by default row = 0 and column = 0?
Or the next line?

Because of this possible confusion I think we should not implement it before this can be answered.

Kim_Panda
Normal user
Posts: 41
Joined: 25 Jan 2021, 17:03

Re: StringConverter.cpp/GetArgvBeginEnd: Bug or feature?

#6 Post by Kim_Panda » 04 Apr 2021, 19:03

Your question:
TD-er wrote: 03 Apr 2021, 22:36 For example what if you have a command to write something on a display.
e.g. writeToDisplay,column,row,text
What will be done when giving this command:

writeToDisplay,,,"This is a text"

Is it by default row = 0 and column = 0?
Or the next line?
I don't know. That depends on the specific functionality of that entity and if it intends to support optional parameters.
Today it will do the same as if you had given:

Code: Select all

writeToDisplay,"This is a text"
and I'd expect, it will return an error, as it cannot convert the text into a column-number. Or it detects that text is provided in the first parameter and does its own default handling and write the text somewhere.

BTW. When I look over today's commands, there are only a hand full , which support optional parameters at all. And probably the current implementation of parseString() is the reason for that and if someone wanted more functionality, he would need to write an own variant of the parseString() function. Or ..., simply there has not been any need for that and I was the first who was astonished about the current limitation in functionality. :shock:

Anyhow, in lack of a general answer to your question, I'll leave it as it is, according to your assessment:
TD-er wrote: 03 Apr 2021, 22:36 Because of this possible confusion I think we should not implement it before this can be answered.

TD-er
Core team member
Posts: 8643
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: StringConverter.cpp/GetArgvBeginEnd: Bug or feature?

#7 Post by TD-er » 04 Apr 2021, 19:31

Just to clarify it a bit more.
I do like the idea of having 'empty' or 'default' parameters, but I'm afraid of the issues it may cause if we introduce inconsistency in how commands are handled.

So either it is added right (including considering all existing commands) or we don't implement it.
Having to keep track of which command supports empty parameters and which one doesn't is really messy and you can't explain it to the users.
Only a handful of commands may benefit from it, so why not create small variants of those commands where you imply some default value.

Post Reply

Who is online

Users browsing this forum: Ahrefs [Bot] and 24 guests