rewrote config file path search #219

Merged
6543 merged 7 commits from crapStone/tea:implement-xdg-specification into master 2020-10-06 13:06:50 +00:00
Contributor
  • added xdg package to get the correct path according to the xdg specification on "all" operating systems
  • removed custom $HOME detection in favour of the xdg implementation

fixes #203

* added `xdg` package to get the correct path according to the xdg specification on "all" operating systems * removed custom `$HOME` detection in favour of the `xdg` implementation fixes #203
crapStone added 1 commit 2020-10-04 21:33:08 +00:00
rewrote config file path search
Some checks failed
continuous-integration/drone/pr Build is failing
1c37438b89
Author
Contributor

Please look very carefully and point out every mistake, missed convention, wrong formatting, ...

This is my first time coding in Go so it's very likely, that i made mistakes and i want to learn this language in oder to make a deep comparison between Go and my currently favourite language Rust.
And because i have to learn it in my uni this semester :)

Please look very carefully and point out every mistake, missed convention, wrong formatting, ... This is my first time coding in Go so it's very likely, that i made mistakes and i want to learn this language in oder to make a deep comparison between Go and my currently favourite language Rust. And because i have to learn it in my uni this semester :)
crapStone added 1 commit 2020-10-04 21:56:27 +00:00
added package xdg to vendor folder
Some checks failed
continuous-integration/drone/pr Build is failing
1f657ea42d
crapStone added 1 commit 2020-10-04 22:04:38 +00:00
fixed vendor folder
All checks were successful
continuous-integration/drone/pr Build is passing
2bf796cc88
noerw requested changes 2020-10-04 22:34:10 +00:00
Dismissed
@ -43,1 +42,3 @@
log.Fatal("Init tea config dir " + dir + " failed")
log.Fatal("Getting fallback config Path failed")
}
if !exists {
Member

handling the err != nil case above in this if clause instead should be more robust

handling the `err != nil` case above in this if clause instead should be more robust
6543 marked this conversation as resolved
@ -82,1 +79,4 @@
func getFallbackPath() string {
dir := filepath.Join(xdg.Home, ".tea")
err := os.MkdirAll(dir, os.ModePerm)
Member

We don't need the MkdirAll() anymore, as this is now a legacy path.
Instead we need to check in GetConfigPath() as a last step if the found path exists, and if not create xdg.ConfigHome + "/tea/config.yml"

We don't need the MkdirAll() anymore, as this is now a legacy path. Instead we need to check in `GetConfigPath()` as a last step if the found path exists, and if not create `xdg.ConfigHome + "/tea/config.yml"`
6543 marked this conversation as resolved
@ -132,3 +132,3 @@
err := LoadConfig()
if err != nil {
log.Fatal("Unable to load config file " + yamlConfigPath)
log.Fatal("Unable to load config file: ", GetConfigPath())
Member

You could make LoadConfig() return the selected path, so we dont need to check all the paths again here. Applies to InitCommand as well

You could make `LoadConfig()` return the selected path, so we dont need to check all the paths again here. Applies to InitCommand as well
6543 marked this conversation as resolved
noerw added the
status/needs-reviews
kind
enhancement
labels 2020-10-04 22:35:12 +00:00
noerw reviewed 2020-10-04 22:39:09 +00:00
Dismissed
@ -83,0 +81,4 @@
dir := filepath.Join(xdg.Home, ".tea")
err := os.MkdirAll(dir, os.ModePerm)
if err != nil {
log.Fatal("Init tea config dir " + dir + " failed")
Member

(with the comment above, this line is obsolete, but for some feedback:) Usually you'd try to return the error as far up the call stack as possible, instead of crashing here. Granted, this codebase doesn't do that properly anyway ¯_(ツ)_/¯

(with the comment above, this line is obsolete, but for some feedback:) Usually you'd try to return the error as far up the call stack as possible, instead of crashing here. Granted, this codebase doesn't do that properly anyway ¯\_(ツ)_/¯
Author
Contributor

So the correct version would be this?

func getFallbackPath() (string, error) {
	dir := filepath.Join(xdg.Home, ".tea")
	err := os.MkdirAll(dir, os.ModePerm)
	if err != nil {
		return dir, err
	}

	return filepath.Join(dir, "tea.yml"), nil
}

Is it convention to return the actual value even if an error occured?
The compiler won't let me return nil insted of dir and sometimes this value is used in the error handling.

So the correct version would be this? ```go func getFallbackPath() (string, error) { dir := filepath.Join(xdg.Home, ".tea") err := os.MkdirAll(dir, os.ModePerm) if err != nil { return dir, err } return filepath.Join(dir, "tea.yml"), nil } ``` Is it convention to return the actual value even if an error occured? The compiler won't let me return `nil` insted of `dir` and sometimes this value is used in the error handling.
Member

It's convention to return the null-value for the given type:
nil is the null-value for pointers (and errors), for strings it is the empty string "".

It's convention to return the null-value for the given type: `nil` is the null-value for pointers (and errors), for strings it is the empty string `""`.
Author
Contributor

Should i use log.Fatal(err) or log.Fatal(err.Error())?
I saw both versions in the code.

Should i use `log.Fatal(err)` or `log.Fatal(err.Error())`? I saw both versions in the code.
Member

Both variants are equivalent, go for the first one

Both variants are equivalent, go for the first one
Member

@crapStone thank you! looks good so far, though the behaviour is not quite correct

While this codebase may give a good intro to golang, it does not really show off the unique perks of the language ;)
Correct formatting is generated via go fmt, there's only one right way

@crapStone thank you! looks good so far, though the behaviour is not quite correct While this codebase may give a good intro to golang, it does not really show off the unique perks of the language ;) Correct formatting is generated via `go fmt`, there's only one right way
noerw removed the
status/needs-reviews
label 2020-10-04 22:50:52 +00:00
6543 added this to the v0.6.0 milestone 2020-10-05 02:01:32 +00:00
crapStone added 1 commit 2020-10-05 12:17:19 +00:00
added changes from conversation
All checks were successful
continuous-integration/drone/pr Build is passing
4374f120ed
Author
Contributor

I should not write code when i'm tired XD.

I should not write code when i'm tired XD.
crapStone added 1 commit 2020-10-05 12:23:53 +00:00
Merge branch 'master' into implement-xdg-specification
All checks were successful
continuous-integration/drone/pr Build is passing
4f74edc750
6543 requested changes 2020-10-05 18:47:09 +00:00
Dismissed
@ -48,0 +44,4 @@
file := filepath.Join(xdg.Home, ".tea", "tea.yml")
exists, _ = utils.PathExists(file)
if exists {
configFilePath = file
Owner

if exist ... return file

if exist ... `return file`
6543 marked this conversation as resolved
@ -48,0 +45,4 @@
exists, _ = utils.PathExists(file)
if exists {
configFilePath = file
} else {
Owner

no need for elese .. because of return file

.. create new config file in configFilePath and returne this value

no need for elese .. because of `return file` .. create new config file in `configFilePath` and returne this value
6543 marked this conversation as resolved
crapStone added 1 commit 2020-10-05 19:50:39 +00:00
fixed next logic error
All checks were successful
continuous-integration/drone/pr Build is passing
6bc9e1f2a0
6543 approved these changes 2020-10-05 19:54:01 +00:00
Dismissed
crapStone added 1 commit 2020-10-05 19:55:42 +00:00
added comment to clarify coding choices
All checks were successful
continuous-integration/drone/pr Build is passing
9c5bd0c354
noerw approved these changes 2020-10-06 10:55:32 +00:00
Dismissed
6543 merged commit c4e2db32b5 into master 2020-10-06 13:06:50 +00:00
Sign in to join this conversation.
No description provided.