In the default case of command line subcommands, stop `fpm` running in time

[zoziha]

To close #727, a small modification:

1. In the default case of command line subcommands (>> fpm), stop fpm running in time.
2. Remove unnecessary references to number_of_rows in fpm.f90.

[urbanjost]

If you are going to stop after the help is displayed in this manner, there are three places, not just one where you want to stop after displaying the text. You only are changing 499:

LINE NUMBER 461 call printhelp(help_text) 499 call printhelp(help_list_dash) 598 CALL PRINTHELP(HELP_TEXT)

A few small changes to append to the HELP_TEXT variable instead of multiple calls to PRINTHELP would allow for another solution where PRINTHELP would only be called at these three locations and then the PRINTHELP procedure would have the STOP in it.

[urbanjost]

I think it might be preferable to replace some of the calls to printhelp with assignments to the variable HELP_TEXT and then change PRINTHELP() to always stop. Perhaps a matter of taste, but I think that ends up with a clearer approach. For example:

diff --git a/src/fpm_command_line.f90 b/src/fpm_command_line.f90
index 88e400c2..5ae6e6b3 100644
--- a/src/fpm_command_line.f90
+++ b/src/fpm_command_line.f90
@@ -494,10 +494,11 @@ contains
             call set_args(common_args // '&
             & --list F&
             &', help_list, version_text)
-            call printhelp(help_list_nodash)
+            help_text=help_list_nodash
             if(lget('list'))then
-               call printhelp(help_list_dash)
+                help_text=[character(len=256) :: help_text, help_list_dash]
             endif
+            call printhelp(help_text)
 
         case('test')
             call set_args(common_args // compiler_args // run_args // ' --', &
@@ -589,11 +590,11 @@ contains
                 elseif(len_trim(cmdarg).eq.0)then
                     write(stdout,'(*(a))')'Fortran Package Manager:'
                     write(stdout,'(*(a))')' '
-                    call printhelp(help_list_nodash)
+                    help_text=[character(len=256) :: help_list_nodash, help_text]
                 else
                     write(stderr,'(*(a))')'<ERROR> unknown subcommand [', &
                      & trim(cmdarg), ']'
-                    call printhelp(help_list_dash)
+                    help_text=[character(len=256) :: help_list_dash, help_text]
                 endif
                 call printhelp(help_text)
             endif
@@ -633,6 +634,7 @@ contains
                write(stdout,'(a)')'<WARNING> *printhelp* output requested is empty'
            endif
         endif
+        stop
     end subroutine printhelp
 
     end subroutine get_command_line_settings

[zoziha]

Thanks for the reminder and suggestion, @urbanjost . I didn't even notice the need to stop at lines 461, 499, 598.

I agree that stopping the program in the printhelp routine would make more sense. Other than that, how should we handle a command like fpm search, does it need to add stop manually: https://github.com/fortran-lang/fpm/blob/1f2831f5a37fe49bd549d6eeafcb091ef9ce6bdb/src/fpm_command_line.f90#L578

[zoziha]

As suggested, I made the corresponding changes:

• [x] Set stop in printhelp;
• [x] Set stop for fpm-XXX commands, like fpm-search;
• [x] Set widest as a constant, and concatenate help_text arrays.

[urbanjost]

The other question to me is whether those messages about the directory should show at all unless the --verbose switch is present. I think they should not. They and all such messages should go to stderr, not stdout; as in earlier versions. Going to stdout breaks or complicates many features, such as the --runner switch. If you use the --list switch or --runner switch to try to just get names that you pipe to another command those messages appear in the pipe as well, polluting it. The change as far as it goes look good to me otherwise. That prevents stops from being scattered around the code and looks much tidier to my eye.

[urbanjost]

• Looked good to me too, but I am not listed as a reviewer. Had not noticed this was not merged yet. I think it should be.