chrisb wrote: ↑Fri Jun 11, 2021 10:13 pm
Great addition. For me all three options are an improvement. For keeping as much as possible in its current state I would go for option 1 or 3.
abdullah wrote: ↑Sat Jun 12, 2021 5:14 am
I like it. Merged. Thanks.
1 or 3 is ok with me. In addition, you may make the new tool the initial default (before the new parameter has a given value).
abdullah wrote: ↑Sat Jun 19, 2021 6:31 am
I found the "-1" in:
"int curRadDiaCons = hGrp->GetInt("CurRadDiaCons", -1);"
misleading for a code reader.
I see that the switch/case will filter this via "default", but still it is unexpected.
Humm. In my own programming paradigm, this is quite standard for 'undefined' (among other depending on the context). I'm not against changing that but can't think about something I'm sure is better. May you have a proposal?
abdullah wrote: ↑Sat Jun 19, 2021 6:31 am
I found the "-1" in:
"int curRadDiaCons = hGrp->GetInt("CurRadDiaCons", -1);"
misleading for a code reader.
I see that the switch/case will filter this via "default", but still it is unexpected.
Humm. In my own programming paradigm, this is quite standard for 'undefined' (among other depending on the context). I'm not against changing that but can't think about something I'm sure is better. May you have a proposal?
abdullah wrote: ↑Sat Jun 19, 2021 11:32 am
What about "2"?
If I am understanding it correctly, we want that when the parameter does not exist, it defaults to "2" i.e. your improvement.
'2' will functionally do the same ATM. My idea was to be more generic in case there is an evolution in the future.
I can update the PR in the coming hours if you think '2' is the way to go. Just tell me.
Edit : just saw you already merged. So I can create a new PR to change if needed, or of course you can feel free to do it by yourself.
abdullah wrote: ↑Sat Jun 19, 2021 11:32 am
What about "2"?
If I am understanding it correctly, we want that when the parameter does not exist, it defaults to "2" i.e. your improvement.
'2' will functionally do the same ATM. My idea was to be more generic in case there is an evolution in the future.
I can update the PR in the coming hours if you think '2' is the way to go. Just tell me.
Edit : just saw you already merged. So I can create a new PR to change if needed, or of course you can feel free to do it by yourself.
How I see it is that no matter where we might be heading in the future, we need a default value, which may be the current one or another one. I am happier with the parameter taking a value that is valid within the code, because the code shows this intent.
It was not my idea to have you change just one value. I did not merge it in the morning because I still had to compile it and run it and I was running out of time. I have now added a commit making the default '2'.
abdullah wrote: ↑Sat Jun 19, 2021 12:01 pm
It was not my idea to have you change just one value. I did not merge it in the morning because I still had to compile it and run it and I was running out of time. I have now added a commit making the default '2'.
I'm very fine with having a shepherd for FC code and having my code reviewed. And I'm fine with just having to change a single value too. Finally it means my code was pretty good.
Thanks for the code
Thanks for the merge. I'll go for the documentation now.