Repository: zyedidia/micro
File: internal/action/command.go (Go)

Code before:
	config.InitRuntimeFiles(true)

	if reloadPlugins {
		config.InitPlugins()
	}

	err := config.ReadSettings()
	if err != nil {
		screen.TermMessage(err)
	} else {
		parsedSettings := config.ParsedSettings()
		defaultSettings := config.DefaultAllSettings()
	outerLoop:
		for k := range defaultSettings {
			if k == "fileformat" {
				// reload should not override auto-detected file format
				continue
			}
			for _, s := range config.LocalSettings {
				if s == k {
					// reload should not override local-only settings
					continue outerLoop
				}
			}
			if _, ok := parsedSettings[k]; ok {
				if err = config.OptionIsValid(k, parsedSettings[k]); err != nil {
					screen.TermMessage(err)
					continue
				}
				err = processGlobalOptionNative(k, parsedSettings[k], false)
			} else {
				err = processGlobalOptionNative(k, defaultSettings[k], false)
			}
			if err != nil {
				screen.TermMessage(err)
			}
		}
	}

	if reloadPlugins {
		err = config.LoadAllPlugins()
		if err != nil {
			screen.TermMessage(err)
		}
	}

	InitBindings()
	InitCommands()

	if reloadPlugins {
		err = config.RunPluginFn("preinit")

Review comment (bug):
So... in an attempt to fix bugs, we are adding more special cases in more places, making the code more complicated and messy (as if it wasn't messy already), and undoubtedly adding more hidden bugs to be found in the future. Why, instead, not stop and think, and analyze existing cases, and try to simplify and clean up and generalize them, not increasing but reducing the number of special cases to remember about, to make the code simpler and cleaner and easier to understand and reason about.

1. Currently (without this PR) option validators are only called from `GetNativeValue()`, so they are only called by `SetOption()` and `SetGlobalOption()`, and not called by `SetOptionNative()` or `SetGlobalOptionNative()` called directly, e.g. by a plugin. So why not fix it properly, i.e. move option validation out of `GetNativeValue()` to a proper place, i.e. to `SetOptionNative()` and `SetGlobalOptionNative()`, so that it is automatically done in both cases?

2. Assuming that we can validate options in `config.ReadSettings()`, it would mean that the settings returned by `config.ParsedSettings()` are always already validated, so we don't need to validate them again here. So it seems we can simplify the code organization around `SetGlobalOptionNative()`: we can just have the standard public `SetGlobalOptionNative()` function which does all it should (i.e. validates, checks for locals, writes to `settings.json`), and a low-level private `setGlobalOptionNative()` function (without any extra args) which does none of that, it just sets the global option, - and call that low-level function here in `reload`. (BTW maybe a better name for it would be e.g. `doSetGlobalOption()`, to make it more distinguishable.)

Code after:
	config.InitRuntimeFiles(true)

	if reloadPlugins {
		config.InitPlugins()
	}

	err := config.ReadSettings()
	if err != nil {
		screen.TermMessage(err)
	} else {
		parsedSettings := config.ParsedSettings()
		defaultSettings := config.DefaultAllSettings()
		for k := range defaultSettings {
			if _, ok := config.VolatileSettings[k]; ok {
				// reload should not override volatile settings
				continue
			}

			if _, ok := parsedSettings[k]; ok {
				err = doSetGlobalOptionNative(k, parsedSettings[k])
			} else {
				err = doSetGlobalOptionNative(k, defaultSettings[k])
			}
			if err != nil {
				screen.TermMessage(err)
			}
		}
	}

	if reloadPlugins {
		err = config.LoadAllPlugins()
		if err != nil {
			screen.TermMessage(err)
		}
	}

	InitBindings()
	InitCommands()

	if reloadPlugins {
		err = config.RunPluginFn("preinit")
		if err != nil {
			screen.TermMessage(err)
		}
		err = config.RunPluginFn("init")
		if err != nil {
			screen.TermMessage(err)
		}
		err = config.RunPluginFn("postinit")
		if err != nil {
			screen.TermMessage(err)