Suggested changes. by bobg · Pull Request #1 · DanWlker/remind

bobg · 2024-07-08T04:55:55Z

Suggested edits, as requested at https://www.reddit.com/r/golang/comments/1dx7hro/a_code_review_request_for_beginner_go_cli_app/

bobg

Overall this code looks very good - it's clear, well-laid-out, and looks correct.

Here are some suggestions about ways to make it more idiomatic, and in some places simpler and/or more efficient.

You should consider adding a bunch of unit tests.

bobg · 2024-07-08T04:56:32Z

 Installation (MacOs only currently):

-    brew install danwlker/remind/remind@0.0.2-alpha
+```sh


Use this kind of Markdown quoting to show example shell commands.

bobg · 2024-07-08T04:56:50Z

+Alternatively, with Go installed:
+
+```sh
+go install github.com/DanWlker/remind@latest


This is the standard way to install Go programs from source, if you have go installed yourself.

bobg · 2024-07-08T04:57:44Z

 module github.com/DanWlker/remind

-go 1.22.4
+go 1.21


According to Mingo, this code doesn't need a newer version of Go than 1.21 in order to compile.

bobg · 2024-07-08T04:58:53Z

-		errAddTodoAndAssociateTo := addTodoAndAssociateTo("", args)
-		if errAddTodoAndAssociateTo != nil {
-			return fmt.Errorf("addTodoAndAssociateTo: %w", errAddTodoAndAssociateTo)
+		if err := addTodoAndAssociateTo("", args); err != nil {


Always use the name err for error variables, except in the rare case when you need to distinguish between multiple error values at the same time.

This name is so idiomatic that anything else is unnecessarily hard to read.

I was wondering about this, if there are multiple errors for a single function, whats the correct way of naming the different errors altogether? Is there a recommended format to stick to?

Just keep reusing err, as I've done here. This is safe as long as you're all finished with one value of err before setting it to a new value.

Of course if you do need to refer to an old error value, you'll need a different name. For example, you might want to accumulate errors with errors.Join, in which case your code will need to look something like this:

result1, err := firstOperation() result2, err2 := secondOperation() err = errors.Join(err, err2) // note, this deals with nils so you don't have to ... return overallResult, err

Or if you prefer:

result1, err := firstOperation() overallErr := err result2, err := secondOperation() overallErr = errors.Join(overallErr, err) ... return overallResult, overallErr

Incidentally, the same thing goes for variables of type context.Context: they should almost always be named ctx, even when you're creating a new one from an old one with something like:

ctx, cancel := context.WithCancel(ctx)

Safe as long as you don't need the old value again, which in practice you seldom do.

bobg · 2024-07-08T05:00:21Z

-	homeRemCurrProExDir, errHomeRemCurrProExDir := shared.GetHomeRemovedWorkingDir()
-	if errHomeRemCurrProExDir != nil {
-		return fmt.Errorf("helper.GetHomeRemovedCurrentProgramExecutionDirectory: %w", errHomeRemCurrProExDir)
+	dir, err := shared.GetHomeRemovedWorkingDir()


Prefer short variable names, which paradoxically improve readability most of the time.

As a rule of thumb, the closer together the earliest and the latest uses of a variable name are, the shorter it can and should be. (Conversely, when uses of a variable are separated by a lot of space, you need to make the name more descriptive.)

bobg · 2024-07-08T05:24:11Z

-func (e *FilePathNotStartsWithHome) Error() string {
-	return fmt.Sprintf("File path %v does not start with: %v", e.FileStr, e.HomeStr)
+func (e NotUnderHomeError) Error() string {
+	return fmt.Sprintf("file path %v does not start with: %v", e.File, e.Home)


Don't capitalize error strings. See https://go.dev/wiki/CodeReviewComments#error-strings

bobg · 2024-07-08T05:27:22Z

+func GetFileContents() ([]RecordEntity, error) {
+	recordFile, err := GetFile()
+	if err != nil {
+		return nil, fmt.Errorf("GetRecordFile: %w", err)


I think this was previously returning []RecordEntity{}. But nil is the better way to express "an empty slice." It works the same way in all the contexts you care about - len, append, etc. - and unlike []RecordEntity{} does not allocate an empty backing array for the slice header to point to.

bobg · 2024-07-08T05:29:52Z

+		return RecordEntity{}, fmt.Errorf("GetDataFolder: %w", err)
+	}
+
+	newFile, err := os.CreateTemp(dataFolder, "*"+config.DefaultDataFileExtension)


This looks like it's going to leak tempfiles. Who/what is responsible for cleaning these up?

i was using this as a way to generate non conflicting file names directly inside the data folder, so in this case it counts as persistent data. Though I think it does not prevent this api from being used incorrectly. Perhaps i should split this into two different functions? Hmmm let me think about it

Ah! Clever. I totally overlooked that you were passing in dataFolder as the first argument. I'm so accustomed to that being "", and for this function being used exclusively for temp files (as the name implies).

In light of that misunderstanding, I suggest adding a comment here to prevent confusion in the next reader of this code - or in yourself when you return to this code years from now!

bobg · 2024-07-08T05:31:09Z

+var editor = "vim"
+
+func init() {
+	if s := os.Getenv("VISUAL"); s != "" {


Use VISUAL in preference to EDITOR when available. See e.g. https://unix.stackexchange.com/questions/4859/visual-vs-editor-what-s-the-difference

bobg · 2024-07-08T05:32:26Z

+	}
+	defer os.Remove(tempFile.Name())
+
+	if _, err := tempFile.Write(data); err != nil {


I think this was previously enclosed in an if len(data) != 0. But that test is unnecessary. If len(data) is 0, the call to tempFile.Write will just be a no-op.

DanWlker · 2024-07-08T11:08:56Z

Hi wow first let me say thanks for taking the time to review it. I will go through it in the next few days. Thanks again

DanWlker · 2024-07-09T08:09:16Z

-		if item != "" {
+	var (
+		todoList []data.TodoEntity
+		sc       = bufio.NewScanner(bytes.NewReader(result))


oh wow this is neat

DanWlker · 2024-07-09T08:15:04Z

-const DEFAULT_CONFIG_FULL_FILE_NAME = string(os.PathSeparator) + DEFAULT_CONFIG_FILE_NAME + "." + DEFAULT_CONFIG_FILE_TYPE
+	DefaultConfigFileType     = "yaml"
+	DefaultConfigFileName     = ".remind"
+	DefaultConfigFullFileName = string(os.PathSeparator) + DefaultConfigFileName + "." + DefaultConfigFileType


Yeah I actually started this thinking of using markdown as the data file type and yaml as the config file type, I think for the config case you're probably right its redundant

DanWlker · 2024-07-09T08:20:06Z

+	"strings"
+
+	"github.com/spf13/viper"
+	"gopkg.in/yaml.v3"


I have considered this library when searching for yaml library, but I saw its last commit was around 2 years ago so I'm not sure if its abandoned or the project is just complete as it is. So i went with the other one

DanWlker · 2024-07-09T08:23:57Z

hmmm this was intended to be a cli app only, I just kept the cmd folder there as that's where cobra added the files when running the cobra-cli add command

DanWlker · 2024-07-09T08:32:06Z

+		return RecordEntity{}, fmt.Errorf("GetDataFolder: %w", err)
+	}
+
+	newFile, err := os.CreateTemp(dataFolder, "*"+config.DefaultDataFileExtension)


i was using this as a way to generate non conflicting file names directly inside the data folder, so in this case it counts as persistent data. Though I think it does not prevent this api from being used incorrectly. Perhaps i should split this into two different functions? Hmmm let me think about it

DanWlker · 2024-07-10T14:31:09Z

+		return fmt.Errorf("in yaml encoding: %w", err)
+	}
+
+	if err := f.Close(); err != nil {


I was wondering why this extra call was here although there was a defer. Then i experimented with returning errors from defer, but I only managed to get it to work if the error is declared in the function signature

This works:

func oneFunc() (err error) { err = fmt.Errorf("error 1") defer func() { err = errors.Join(err, fmt.Errorf("error 2")) }() return err }

This does not work:

func oneFunc() error { err := fmt.Errorf("error 1") defer func() { err = errors.Join(err, fmt.Errorf("error 2")) }() return err }

Is this a good way to go about it?

Yes it is. If you want a deferred function to affect the outer function's return value, then as you discovered, you need to use named result parameters. (See https://go.dev/wiki/CodeReviewComments#named-result-parameters.) And errors.Join is a good way to preserve both the error that made your function return (if any), and the error encountered while cleaning up.

Most of the time I prefer how simple defer f.Close() looks, and when the surrounding function returns due to an error, I'm OK losing any error from the Close call. But when the function is about to return normally, I prefer to capture any error from Close, which is why there's also an explicit (non-deferred) call near the end.

(In that case, the redundant deferred Close still runs, but it's a no-op since the file's already closed, and conveniently the "already-closed" error is disregarded.)

DanWlker · 2024-07-10T15:32:32Z

+		return "", fmt.Errorf("os.UserHomeDir: %w", err)
+	}
+
+	rel, err := filepath.Rel(home, filePathWithHome)


Note to self: This is neat

Suggested changes.

9ff24b5

bobg commented Jul 8, 2024

View reviewed changes

bobg marked this pull request as draft July 8, 2024 05:36

DanWlker reviewed Jul 10, 2024

View reviewed changes

Conversation

bobg commented Jul 8, 2024

Uh oh!

bobg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobg Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobg Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanWlker commented Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bobg Jul 9, 2024 •

edited

Loading

bobg Jul 8, 2024 •

edited

Loading

DanWlker commented Jul 8, 2024 •

edited

Loading