Author is a required block in the global config. Should it be?

It is possible to leave empty (or indeed have no config file at all) which implies it could be an Option…

I’ve made Author optional if that’s something you think is a good idea. Cheers.

+1

Also linking back to #861 here which solves this in a different way. :)

I’ve applied the patches from #861 to main, can these still be applied?

These definitely conflict now. The main question to answer is: Do we want the Author block to be an Option like all other settings?

If we do, I’ll remake the change on the latest. There has been a lot of refactoring since this was made.

Do we want the Author block to be an Option like all other settings?

I think the question should rather be: Should an empty author section mean something different than no author section at all? Because otherwise, you have two variants that mean the same thing (None vs default author struct) and that you’re always going to convert into the latter anyway via an otherwise unnecessary unwrap_or_default() or something comparable, so distinguishing them is pointless.

@dblsaiko So you always generate a default with your change? That seems fine to me.

NOTE: When I originally made this change, I also noticed that the “default” Author was different in a couple of places. I don’t know if that was intentional or a potential issue.

i.e. repair.rs populates Author.username where mod.rs only uses Author::default().

Additionally, if we unwrap and use defaults for other config options in other places, would we benefit from coalescing all the config options to their defaults directly so we always have genuine values?

Yep, the default is all empty fields. It’s fine since everything in Author is options as well.

Additionally, if we unwrap and use defaults for other config options in other places, would we benefit from coalescing all the config options to their defaults directly so we always have genuine values?

If you mean actually filling in real default values instead of leaving the values as options, that depends on whether the struct is ever saved back to disk or otherwise serialized (IIRC it is marked #[derive(Serialize)]). Because then every time it would save the file, it’d fill in the default values for each of the config values instead of leaving them unset. Getting the actual value would probably be better done via a method on the Config struct that handles that.

repair.rs populates Author.username

in Complete::from_old_format? From a quick look that actually does look like it’s eventually writing the config file, so yeah, you will want to keep the options in the struct around. But that seems intentional and fine since it’s setting the username from the old configuration.

I think what I’m driving at or thinking about is a common approach to all the configuration options, if it makes sense to do so…

The problem this PR originally set out to fix was that if the Author block is missing the whole config was being thrown away. If that’s already fixed by your PRs then I think whatever else comes of this discussion is not urgent. 😀