Fix #404: nil map error when reading env file #405

2023-11-16T15:26:27Z

infinoid commented

2023-11-16 15:26:27 +00:00

(Migrated from gitea.com)

No description provided.

wolfogre (Migrated from gitea.com) reviewed 2023-11-17 06:03:17 +00:00

wolfogre commented

2023-11-21 06:06:09 +00:00

(Migrated from gitea.com)

I think it would be more clear to fix it by:

	if cfg.Runner.EnvFile != "" {
		if stat, err := os.Stat(cfg.Runner.EnvFile); err == nil && !stat.IsDir() {
			envs, err := godotenv.Read(cfg.Runner.EnvFile)
			if err != nil {
				return nil, fmt.Errorf("read env file %q: %w", cfg.Runner.EnvFile, err)
			}
+            if cfg.Runner.Envs == nil {
+            	cfg.Runner.Envs = map[string]string{}
+            }
			for k, v := range envs {
				cfg.Runner.Envs[k] = v
			}
		}
	}

I think it would be more clear to fix it by: ```diff if cfg.Runner.EnvFile != "" { if stat, err := os.Stat(cfg.Runner.EnvFile); err == nil && !stat.IsDir() { envs, err := godotenv.Read(cfg.Runner.EnvFile) if err != nil { return nil, fmt.Errorf("read env file %q: %w", cfg.Runner.EnvFile, err) } + if cfg.Runner.Envs == nil { + cfg.Runner.Envs = map[string]string{} + } for k, v := range envs { cfg.Runner.Envs[k] = v } } } ```

infinoid commented

2023-11-21 10:05:01 +00:00

(Migrated from gitea.com)

+            if cfg.Runner.Envs == nil {
+            	cfg.Runner.Envs = map[string]string{}
+            }

Yeah, that's an option, I mentioned it in #404. I decided not to do it that way because then, the map is only initialized when EnvFile is set. I wondered if there would be other problems with the nil map later on. I chose to initialize it up-front because that way it is always initialized.

So I'm curious, why is it clearer that way? I think that "map is always valid; sometimes written into by yaml, sometimes written into by envfile" is clearer than "sometimes initialized by yaml, sometimes initialized by envfile, sometimes nil". I guess by "clearer" I mean "requires less guesswork".

But you're much more familiar with the project than I am. So if you still think the conditional approach is better, I will happily update the PR.

> ```diff > + if cfg.Runner.Envs == nil { > + cfg.Runner.Envs = map[string]string{} > + } > ``` Yeah, that's an option, I mentioned it in #404. I decided not to do it that way because then, the map is only initialized when EnvFile is set. I wondered if there would be other problems with the nil map later on. I chose to initialize it up-front because that way it is *always* initialized. So I'm curious, why is it clearer that way? I think that "map is always valid; sometimes written into by yaml, sometimes written into by envfile" is clearer than "sometimes initialized by yaml, sometimes initialized by envfile, sometimes nil". I guess by "clearer" I mean "requires less guesswork". But you're much more familiar with the project than I am. So if you still think the conditional approach is better, I will happily update the PR.

wolfogre commented

2023-11-22 07:02:39 +00:00

(Migrated from gitea.com)

Just my opinion.

When developers see cfg := &Config{Runner: Runner{Envs: map[string]string{}}}, the question may come to their mind: why should we initialize the Envs map? If I want to add a new map field, should I initialize it too?
I think it could be clearer:

+            if cfg.Runner.Envs == nil {
+            	cfg.Runner.Envs = map[string]string{}
+            }
			for k, v := range envs {
				cfg.Runner.Envs[k] = v
			}

because it's easy to understand that we initialize Envs to prepare it for adding more items and to avoid panic.

If both envs and env_file are empty, I think it's OK to keep Envs as nil.

Just my opinion. When developers see `cfg := &Config{Runner: Runner{Envs: map[string]string{}}}`, the question may come to their mind: why should we initialize the `Envs` map? If I want to add a new map field, should I initialize it too? I think it could be clearer: ```diff + if cfg.Runner.Envs == nil { + cfg.Runner.Envs = map[string]string{} + } for k, v := range envs { cfg.Runner.Envs[k] = v } ``` because it's easy to understand that we initialize `Envs` to prepare it for adding more items and to avoid panic. If both `envs` and `env_file` are empty, I think it's OK to keep `Envs` as nil.

infinoid commented

2023-11-22 10:53:02 +00:00

(Migrated from gitea.com)

Ok, thanks. I have updated the PR.

Your opinion is interesting, thanks for sharing it. I personally always initialize maps in all cases; I see it as just part of the overhead of having a map. That way I never have to worry about nils later on.

So when I see cfg := &Config{}, the question comes to my mind: "why wasn't the Envs map initialized? Is that going to crash later when someone writes to it?" But now I see that this approach is not universal.

Ok, thanks. I have updated the PR. Your opinion is interesting, thanks for sharing it. I personally always initialize maps in all cases; I see it as just part of the overhead of having a map. That way I never have to worry about `nil`s later on. So when I see `cfg := &Config{}`, the question comes to my mind: "why wasn't the `Envs` map initialized? Is that going to crash later when someone writes to it?" But now I see that this approach is not universal.

wolfogre (Migrated from gitea.com) approved these changes 2023-11-24 01:43:52 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: hiifong/act_runner#405