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
}
}
}
```
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.
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.
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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
I think it would be more clear to fix it by:
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.
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 theEnvsmap? If I want to add a new map field, should I initialize it too?I think it could be clearer:
because it's easy to understand that we initialize
Envsto prepare it for adding more items and to avoid panic.If both
envsandenv_fileare empty, I think it's OK to keepEnvsas nil.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 theEnvsmap initialized? Is that going to crash later when someone writes to it?" But now I see that this approach is not universal.