szaghi / FLAP

Fortran command Line Arguments Parser for poor people
150 stars 34 forks source link

Default values of group switches when group not given #71

Closed milancurcic closed 7 years ago

milancurcic commented 7 years ago

Hi Stefano,

What is the current behavior for default values of switches that belong to a group, but group is not invoked? See example below:

program testcli

use flap,only:command_line_interface

implicit none

integer :: error
type(command_line_interface) :: cli

logical :: switch_value_domain
logical :: switch_value_grid
logical :: switch_value_spectrum

! initialize the CLI
call cli % init()

! define sub-commands
call cli % add_group(group='new',description='create new instance')

! define command line arguments
call cli % add(group='new',switch='--spectrum',switch_ab='-s',&
               help='Create new spectrum',required=.false.,def='.false.',&
               act='store_true',error=error)

call cli % add(group='new',switch='--domain',switch_ab='-d',&
               help='Create new domain',required=.false.,def='.false.',&
               act='store_true',error=error)

call cli % add(group='new',switch='--grid',switch_ab='-g',&
               help='Create new grid',required=.false.,def='.false.',&
               act='store_true',error=error)

call cli % get(group='new',switch='--spectrum',val=switch_value_spectrum)
call cli % get(group='new',switch='--domain',val=switch_value_domain)
call cli % get(group='new',switch='--grid',val=switch_value_grid)

write(*,*)'spectrum = ',switch_value_spectrum
write(*,*)'domain   = ',switch_value_domain
write(*,*)'grid     = ',switch_value_grid

endprogram testcli

If I invoke the program with the new sub-command, the behavior is as expected:

$ ./test-cli new
 spectrum =  F
 domain   =  F
 grid     =  F
$ ./test-cli new -s
 spectrum =  T
 domain   =  F
 grid     =  F
$ ./test-cli new -d
 spectrum =  F
 domain   =  T
 grid     =  F
$ ./test-cli new -g
 spectrum =  F
 domain   =  F
 grid     =  T

However, if I invoke the program without the new sub-command, results are what I consider astonishing:

$ ./test-cli
 spectrum =  F
 domain   =  T
 grid     =  F

Based on the above, it looks like the logical variables that I use remain uninitialized, or cli % parse does something weird to them.

Is this the expected behavior? I would expect that even if I don't issue the new sub-command, these switches would get initialized, and set to their default values (.false.).

victorsndvg commented 7 years ago

Hi @milancurcic ,

I think that you must initialize your variables and be sure that a particular group was passed before getting its switch values:

logical :: switch_value_domain = .false.
logical :: switch_value_grid = .false.
logical :: switch_value_spectrum = .false.
...
if (cli%run_command(group='new')) then
     call cli % get(group='new',switch='--spectrum',val=switch_value_spectrum)
     call cli % get(group='new',switch='--domain',val=switch_value_domain)
     call cli % get(group='new',switch='--grid',val=switch_value_grid)
endif
...

see: https://github.com/szaghi/FLAP/wiki/CLI-Method-run_command

szaghi commented 7 years ago

@milancurcic @victorsndvg

Dear Milan, I am sorry my delay (I am quite busy, the first-test day is coming...), I missed this issue.

Probably Victor is right, but I have to admit that probably I simply have not considered your scenario, that is an under-look due to my lazyness... Later today I'll try to do my home-work :smile:

Do you have any preferences on how FLAP should behaves in such a situation?

Cheers.

victorsndvg commented 7 years ago

I don't know if @milancurcic agree with me, but what I expect while getting the value of a non-passed switch is to get the default value.

For me, the same behaviour with non-passed groups (and its switches) is the most coherent option.

@milancurcic and @szaghi , Do you agree?

szaghi commented 7 years ago

@victorsndvg Indeed, this is my first idea, then I thought that maybe some sort of warnings could be liked: group aka command are intended to trigger specific action, it is possible that querying default values of non invoked command could be a logic error meaning an OT action... I see a very little difference with default value of non passed CLAs, but little does not mean null :smile:

victorsndvg commented 7 years ago

Ok, Maybe I was blind because of my previous experience with this situation. When I found this scenario I expect to get the default value for the CLAs ...

Probably yours is the most coherent choice, and of course is valid for me!

szaghi commented 7 years ago

@milancurcic @victorsndvg

Milan, Victor, in the new v1.1.7 I add a patch that seems to solve the bug (but I did not add the warning until you confirm me you like it). Aside the patch, I add a new test test_group (exploiting the cool fortran_tester library). The test output is

tefano@zaghi(02:32 PM Tue Oct 25) on feature/group-default-values-bug-fix [+!?]
~/fortran/FLAP 17 files, 152Kb
→ ./exe/test_group
 test_group
 spectrum =  F
 domain   =  F
 grid     =  F
 test_group new -s
 spectrum =  T
 domain   =  F
 grid     =  F
 test_group new -d
 spectrum =  F
 domain   =  T
 grid     =  F
 test_group new -g
 spectrum =  F
 domain   =  F
 grid     =  T
 fortran_tester:           0  error(s) for          12 test(s)
 fortran_tester: all tests succeeded

Note that I am exploiting the parse fake call, passing the fake arguments directly into the test, thus I can avoid to call 4 times the test itself

It seems to behave as Victor suggested. Let me know if this is ok for you.

Cheers.

milancurcic commented 7 years ago

@szaghi @victorsndvg

Thank you both for your responses and thank you Stefano for the fix.

I find it least surprising (most expected) for the switch to take its default value if its group has not been invoked. My reasoning is that when invoking cli % get(), the val parameter (intent(inout)) must have some value set inside, rather than remain undefined. In other words, I can't think of a situation where a getter should return a value of an uninitialized variable. If the variable is uninitialized at the time of the getter invocation, it should be initialized by the getter.

I think having a warning for this corner case is desirable.

szaghi commented 7 years ago

@milancurcic

Hi Milan, yes I agree with you. My (pedantic/maniac) point is only related to my (distorted) view of the world... I meant the group facility as something a little more on-user-shoulder option than a simple (non grouped) CLA%get. If I try to query a group-cla but the group is not explicitly invoked, me feel was that something is going wrong with the UI usage... this was the reason why I had wrapped the %get by a is_group_called: the fix simply removes this (paranoiac) check thus even if the group is not explicitly invoked the default value is returned.

I will try to think is adding a new initialization option like verbose or debug will help to trigger warnings like this.

Cheers.