Set error status when help is printed

[joshbax189]

This is an attempt to break down #275 into smaller chunks.

Many commands do not set a non-zero exit code when printing a help message , e.g. eask eval is 1, but eask exec is 0.
Both should exit with 1.

Other fixes:

• some grammar/style improvements in the help messages
• fixed missing help message for eask analyze

Comments
Initially I thought to make eask-help always call eask--exit, but some help messages are just informational and should exit with 0, e.g. eask link add prints a message after it adds a link. Likely more would be in this category too, so eask-help should probably not change.

For now I added the new function eask-help-error, but I could equally well inline it too:

;; within exec.el
(eask-help "core/exec")
(eask--exit)

this would avoid introducing a new function. Which do you prefer?

Tests
This removes some FIXMEs from the new tests in #275

I can either move the tests here into the appropriate folders, or rebase #275.

I think it makes sense to move the tests, so that #275 gets smaller, but it would involve adding the script for the should_error helper in this PR.

[jcs090218]

Instead of creating another function, let's try add another parameter in eask-help?

- (defun eask-help (command)
+ (defun eask-help (command &optional flag-error)

Or even better, pass the exit code:

+ (defun eask-help (command &optional exit-code)
   ... in side the function ...
+  (eask--exit exit-code)  ; the default return 0. See below changes:
  )

- (defun eask--exit (&rest _) "Send exit code." (kill-emacs 1))
+ (defun eask--exit (&optional exit-code &rest _) 
+   "Send EXIT-CODE." 
+   (kill-emacs (or exit-code 0)))

Make sure the default output of exit code is 1: (eask--exit is only used in this function)

(defun eask--trigger-error ()
  "Trigger error event."
  (when (and (not eask--ignore-error-p)
             (not (eask-checker-p)))  ; ignore when checking Eask-file
    (if (eask-allow-error-p)  ; Trigger error at the right time
        (add-hook 'eask-after-command-hook #'eask--exit)
-      (eask--exit))))
+      (eask--exit 1))))

---

Then in other places:

- (eask-help-error "...")
+ (eask-help "..." 1)

WDYT? 🙂

[joshbax189]

I tried doing the extra parameter in eask-help originally, but it seemed wrong for a couple of reasons:

• most uses of eask-help do exit with 1, strongly suggesting the default should be 1 not 0
• it's not clear when reading the function call what 0 or 1 should mean
• other components may set an exit code too, e.g. yargs when missing a required param

I think inline is the most clear and flexible solution.

I like the extra option to exit-code though, I'll add that just as a bonus.

[jcs090218]

• most uses of eask-help do exit with 1, strongly suggesting the default should be 1 not 0

Oh, yeah. Sorry, I wasn't thinking clear enough. It should probably be 1 instead of 0. 👍

• it's not clear when reading the function call what 0 or 1 should mean

Yeah, there's no standard yet. It might be a good idea to start specify the exit code. 🤔

Here's what I have in mind:

(defconst eask--exit-code
  `((success . 0)
    (failure . 1)
    ...  and more ...

• other components may set an exit code too, e.g. yargs when missing a required param

This is probably handled by Yargs itself, so I assumed there's no need to take care of it. 🤔Unless I am missing something?

[joshbax189]

Yeah, there's no standard yet. It might be a good idea to start specify the exit code

Nice! For now though eask--exit keeps the same meaning as before and is only ever called without any args. I'll keep it in mind when looking at #275.

This is probably handled by Yargs itself, so I assumed there's no need to take care of it.

Yeah, my point was that there might be some cases where eask-help shouldn't exit at all, so it's better to keep the meaning of eask-help as just "print a formatted message".

[joshbax189]

Ok, using the version you gave lower down. Thanks for the detailed feedback!

[jcs090218]

For now I added the new function eask-help-error, but I could equally well inline it too:

I would say... make it inline first. 🤔 We can change it later if we need to! See more information in the review! :)

I can either move the tests here into the appropriate folders, or rebase #275.

I think it makes sense to move the tests, so that #275 gets smaller, but it would involve adding the script for the should_error helper in this PR.

I'd prefer to include the tests in this PR so I can review them and see the results simultaneously, ensuring everything works correctly. 😋 Adding the needed script is totally fine!

[joshbax189]

I've put the new tests into the corresponding directories under test/command, but grouped the new tests into the Help-Exit CI job to avoid creating heaps of new jobs which test only a single command.

[jcs090218]

I would prefer to test only the latest version since this is unlikely to break with different Emacs versions. WDYT? :)

[joshbax189]

Agree. And just ubuntu as well?

[jcs090218]

And just ubuntu as well?

I'll test it on all operating systems since it's likely something that could easily break.

For reference:

cli/.github/workflows/search.yml

Lines 31 to 33 in 999ce93

	os: [ubuntu-latest, macos-latest, windows-latest]
	emacs-version:
	- 30.1

[joshbax189]

Ok changed back to test all OS'

[jcs090218]

Initially I thought to make eask-help always call eask--exit, but some help messages are just informational and should exit with 0, e.g. eask link add prints a message after it adds a link. Likely more would be in this category too, so eask-help should probably not change.

Sorry, I wasn't being clear enough in the previous post. 😓 I have now fully understand this issue after re-read your comment.

I would still prefer adding a parameter after eask-help, but something more precise:

(defun eask-help (command &optional print-or-exit-code)
  "Show COMMAND's help instruction.

When the optional variable PRINT-OR-EXIT-CODE is a number, it will exit with
that code.  Set to non-nil would just print the help message without sending
the exit code.  The default value `nil' will be replaced by `1'; therefore
would send exit code of `1'."
  (let* ((command (eask-2str command))  ; convert to string
         (help-file (concat eask-lisp-root "help/" command))
         ;; The default exit code is `1' since `eask-help' prints the help
         ;; message on user error 99% of the time.
         ;;
         ;; TODO: Later replace exit code `1' with readable symbol after
         ;; the exit code has specified.
         (print-or-exit-code (or print-or-exit-code 1)))
    (if (file-exists-p help-file)
        (with-temp-buffer
          (insert-file-contents help-file)
          (unless (string-empty-p (buffer-string))
            (let ((buf-str (eask--msg-displayable-kwds (buffer-string))))
              (erase-buffer)
              (insert buf-str))
            (eask--help-display))
          ;; Exit with code if needed
          (cond ((numberp print-or-exit-code)
                 (eask--exit print-or-exit-code))
                (t )))  ; Don't exit with anything else.
      (eask-error "Help manual missig %s" help-file))))

To use it:

(eask-help "xxx")  ; exit with 1 by default
(eask-help "xxx" 2)  ; exit with 2
(eask-help "xxx" t)  ; print only

LMKWYT? 😃

---

BTW, the CI is failing. Is there a way to fix it? 🤔

And again, thank you so much for spending so much time on this! 🚀

[joshbax189]

LMKWYT?

I think it's a less good design because you need an entire paragraph in the eask-help docstring to explain what the new parameter does. Before, the docstrings for eask-help and eask--exit were perfectly descriptive with a single line-- the functions do just what they say! You are paying the price of design complexity to save a couple of extra lines of code here and there 😱

BTW, the CI is failing. Is there a way to fix it?

I'll see what I can do

[jcs090218]

I think it's a less good design because you need an entire paragraph in the eask-help docstring to explain what the new parameter does. Before, the docstrings for eask-help and eask--exit were perfectly descriptive with a single line-- the functions do just what they say! You are paying the price of design complexity to save a couple of extra lines of code here and there 😱

Here are my reasons: 🤔

• eask-help is designed to flag the exit code. I reviewed all instances of eask-help, and the only case where an explicit argument is required is in eask link add, which needs t or a more readable option like :print-only.
• This approach is quite common in Elisp code (though I’m not arguing whether it is good or bad).
• It makes future adjustments easier, as it eliminates the need to manually check every file to determine whether eask--exit should be added.

Overall, this choice is primarily based on maintainability. 🤔

I'll see what I can do

I saw you have already fixed it! Thank you! 🚀

[jcs090218]

Awesome! Thank you for working on this! I greatly appreciate your effort! 😄 🥳 🎉