~taiite/public-inbox

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
5 2

[PATCH senpai] Use log.Fatalln instead of Panicln

Details
Message ID
<20210429100635.15449-1-yyp@disroot.org>
DKIM signature
pass
Download raw message
Patch: +3 -3
It's generally preferred to do an os.Exit(1), which is what log.Fatalln
does, instead of a panic.
---
 cmd/senpai/main.go | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/cmd/senpai/main.go b/cmd/senpai/main.go
index d428ccf..6eb49b3 100644
--- a/cmd/senpai/main.go
+++ b/cmd/senpai/main.go
@@ -28,21 +28,21 @@ func main() {
	if configPath == "" {
		configDir, err := os.UserConfigDir()
		if err != nil {
			log.Panicln(err)
			log.Fatalln(err)
		}
		configPath = path.Join(configDir, "senpai", "senpai.yaml")
	}

	cfg, err := senpai.LoadConfigFile(configPath)
	if err != nil {
		log.Panicln(err)
		log.Fatalln(err)
	}

	cfg.Debug = cfg.Debug || debug

	app, err := senpai.NewApp(cfg)
	if err != nil {
		log.Panicln(err)
		log.Fatalln(err)
	}
	defer app.Close()

-- 
2.31.1
Details
Message ID
<20210429132321.23160279@vroom.localdomain>
In-Reply-To
<20210429100635.15449-1-yyp@disroot.org> (view parent)
DKIM signature
pass
Download raw message
On Thu, 29 Apr 2021 13:06:35 +0300, Alexey Yerin wrote:
> It's generally preferred to do an os.Exit(1), which is what
> log.Fatalln does, instead of a panic.

IMO it's OK for "os.UserConfigDir" and "senpai.NewApp" to call "panic"
on error.  They both should not fail, and "panic" also exits the
program with a non-zero exit code and provides useful debugging info.

The error handling for "senpai.LoadConfigFile" however needs more than
just a call to "log.Fatal", especially for new users.  Maybe print the
error and then a message saying senpai needs a configuration file by
default?  In the future there might be command-line options to specify
the nickname, the server address, etc.
Details
Message ID
<CB0DA4QVL11D.26UO5DKI9EM6H@desktop>
In-Reply-To
<20210429132321.23160279@vroom.localdomain> (view parent)
DKIM signature
pass
Download raw message
On Thu Apr 29, 2021 at 2:23 PM MSK, Hubert Hirtz wrote:
> On Thu, 29 Apr 2021 13:06:35 +0300, Alexey Yerin wrote:
> > It's generally preferred to do an os.Exit(1), which is what
> > log.Fatalln does, instead of a panic.
>
> IMO it's OK for "os.UserConfigDir" and "senpai.NewApp" to call "panic"
> on error. They both should not fail, and "panic" also exits the
> program with a non-zero exit code and provides useful debugging info.

Yeah, that makes sense.

> The error handling for "senpai.LoadConfigFile" however needs more than
> just a call to "log.Fatal", especially for new users. Maybe print the
> error and then a message saying senpai needs a configuration file by
> default? In the future there might be command-line options to specify
> the nickname, the server address, etc.

Maybe we can see what error is reported and print a helpful advise like
"make sure your configuration is properly formatted" or "the
configuration file at ~/.config/senpai/senpai.yml is required". What do
you think?
Details
Message ID
<20210430102829.0dbedea6@vroom.localdomain>
In-Reply-To
<CB0DA4QVL11D.26UO5DKI9EM6H@desktop> (view parent)
DKIM signature
pass
Download raw message
On Thu, 29 Apr 2021 20:00:57 +0300, Alexey Yerin wrote:
> > The error handling for "senpai.LoadConfigFile" however needs more
> > than just a call to "log.Fatal", especially for new users. Maybe
> > print the error and then a message saying senpai needs a
> > configuration file by default? In the future there might be
> > command-line options to specify the nickname, the server address,
> > etc.  
> 
> Maybe we can see what error is reported and print a helpful advise
> like "make sure your configuration is properly formatted" or "the
> configuration file at ~/.config/senpai/senpai.yml is required". What
> do you think?

Pushed e6df23b3f9 which should cover this, config.go had some other
error handling issues.

https://git.sr.ht/~taiite/senpai/commit/e6df23b3f95433141ecacd0219230e7fa1034ef9
Details
Message ID
<CB0XMG1YM51D.HICSMK1UAUNA@desktop>
In-Reply-To
<20210430102829.0dbedea6@vroom.localdomain> (view parent)
DKIM signature
pass
Download raw message
On Fri Apr 30, 2021 at 11:28 AM MSK, Hubert Hirtz wrote:
> On Thu, 29 Apr 2021 20:00:57 +0300, Alexey Yerin wrote:
> > > The error handling for "senpai.LoadConfigFile" however needs more
> > > than just a call to "log.Fatal", especially for new users. Maybe
> > > print the error and then a message saying senpai needs a
> > > configuration file by default? In the future there might be
> > > command-line options to specify the nickname, the server address,
> > > etc.  
> > 
> > Maybe we can see what error is reported and print a helpful advise
> > like "make sure your configuration is properly formatted" or "the
> > configuration file at ~/.config/senpai/senpai.yml is required". What
> > do you think?
>
> Pushed e6df23b3f9 which should cover this, config.go had some other
> error handling issues.
>
> https://git.sr.ht/~taiite/senpai/commit/e6df23b3f95433141ecacd0219230e7fa1034ef9

It looks good to me, though it would be better to use
fmt.Fprintf(os.Stderr, ...) there to print to standard error.
Details
Message ID
<20210501135957.7879f7ec@vroom.localdomain>
In-Reply-To
<CB0XMG1YM51D.HICSMK1UAUNA@desktop> (view parent)
DKIM signature
pass
Download raw message
On Fri, 30 Apr 2021 11:57:24 +0300, Alexey Yerin wrote:
> It looks good to me, though it would be better to use
> fmt.Fprintf(os.Stderr, ...) there to print to standard error.

Indeed. Done in
https://git.sr.ht/~taiite/senpai/commit/10d6a2390ff21a248773ffc7182a89754104e68c
Reply to thread Export thread (mbox)