Cleaning up eval code idea

[Michael-Gallo]

Hello

Currently the setExpr eval function is an over 900 line long switch statement
https://github.com/gokcehan/lf/blob/master/eval.go#L17

In the vast majority of cases the implementation is either exactly the same as or a lightly modified version of other options and this creates a lot of repetition. The string ": value should be empty, 'true', or 'false'" alone shows 30 results in this function. This seems really difficult to maintain and likely to result in errors/typos.

I believe it makes more sense to create a map of string: functions. I have a proof of concept for how I can see this working on my fork. Options that need special logic can have their own functions. I would just like to know what the team thinks of the idea before I put the effort in to fully remove the switch.

I know function calls are not free in Go ,but that at scale a map lookup is more performant than a switch, if this is a concern I can try to write some benchmarks.

[joelim-work]

I have also noticed the large amounts of repetition, which increases the chances of typos when adding new options. I'm not so concerned with performance, since users will usually only set about 10-20 options, and this will only impact startup times. My only concern is the additional complexity involved (i.e. creating another method of setting options, which creates another layer of abstraction and can only handle about half of the options).

I still think this is worth pursuing though, I wrote up my own implementation just now which looks like this:

func applyBoolOpt(opt *bool, e *setExpr) error {
	switch {
	case strings.HasPrefix(e.opt, "no"):
		if e.val != "" {
			return fmt.Errorf("%s: unexpected value: %s", e.opt, e.val)
		}
		*opt = false
	case strings.HasSuffix(e.opt, "!"):
		if e.val != "" {
			return fmt.Errorf("%s: unexpected value: %s", e.opt, e.val)
		}
		*opt = !*opt
	default:
		switch e.val {
		case "", "true":
			*opt = true
		case "false":
			*opt = false
		default:
			return fmt.Errorf("%s: value should be empty, 'true', or 'false'", e.opt)
		}
	}

	return nil
}

func (e *setExpr) eval(app *app, args []string) {
	switch e.opt {
	case "anchorfind", "noanchorfind", "anchorfind!":
		if err := applyBoolOpt(&gOpts.anchorfind, e); err != nil {
			app.ui.echoerr(err.Error())
			return
		}
		// do anything else if necessary
	case "autoquit", "noautoquit", "autoquit!":
		if err := applyBoolOpt(&gOpts.autoquit, e); err != nil {
			app.ui.echoerr(err.Error())
			return
		}
	// remaining options
	}
}

[Michael-Gallo]

On this only handling about half of the options: The map idea is intended to completely remove the switch statement in eval and make that method completely unconcerned with what options exist. The useOption function makes the map unconcerned with the type of the global option being set

[joelim-work]

I think this approach works better when all the cases do the same thing (e.g. set a global option). But some cases also involve side effects, such as calling app.nav.sort(), enabling/disabling mouse events, etc.

The problem is that it's hard for me to judge how effective this approach is if you only apply it to the simple cases, as what you have currently implemented. If you can implement every single option with this map approach, I will take another look.

[Michael-Gallo]

Finally got around to porting over every option

https://github.com/Michael-Gallo/lf/blob/eval-rework/eval.go#L17

As is, with very little polish or proofreading it knocks 300 lines off of the file on net and there's probably a way to reuse some of this for the 168 line switch for the *setLocalExpr eval

[joelim-work]

Yeah, it's as I expected - the original useOption function can't handle 'special' cases, so you needed to add more customized functions like useMouseOption and useOptionWithCleanup, the latter of which accepts a cleanup function as a parameter. At least it works.

[joelim-work]

Tagging @gokcehan, I suspect you will want to review this idea. Also @laktak might be interested after having to implement the hidecursorinactive option in #965.

[gokcehan]

Code cleanups are always welcome. I have looked at the patch and it seems good to me. Thank you @Michael-Gallo for working on this and @joelim-work for the discussion. Feel free to send it as a PR whenever you like.

Some small questions:

• Is there or should there be an order of options? (e.g. alphabetic order or three boolean states altogether)
• Is there an easy way to combine three boolean states as a single entry in the map somehow? (e.g. checking if the argument starts with no or endswith !)
• Is it possible to get rid of the manual listing of options names in our completion code? (e.g. listing keys in this new map at initialization)
• It it possible to use a similar approach to clean our command evaluation code?

[Michael-Gallo]

1. I think I like the idea of grouping first by data type (booleans together) and then alphabetically by basename (and by basename, I mean nomouse would be considered the same as mouse)
2. It should be possible, I don't know if the additional abstraction will make the code less readable though. What I believe I would need to do is take every option, check for no as a prefix and ! as a suffix, remove them and turn them into some sort of flag that's used by optionFunc
3. Sure thing, we would need to add a function like this to get the keys out of the map though

func getStringKeysFromMap[T any](inputMap map[string]T) []string {
	keys := make([]string, len(inputMap))
	i := 0
	for key := range inputMap {
		keys[i] = key
		i++
	}
	return keys
}

4. It should be