Eval rework

[Michael-Gallo]

Simplifies the 900 line switch statement for the setExpr.eval method by using a map

See #1552

[joelim-work]

Thanks for working on this.

I think this should work, but I'm still not sure about this approach in general (maybe because I'm not used to this kind of pattern). There seems to be a fair amount of code required to wrap the (groups of) functionality just so they can be stored in a map. TBH I'm not actually sure what the benefit of replacing the switch statement with a map is - the only thing I can think of is not having to repeat the option names in complete.go, and even then I don't think it's a significant issue.

I was wondering whether you would be interested in an alternate approach which keeps the current structure of the eval function but with the option processing logic extracted into a helper function. The code is in https://github.com/joelim-work/lf/tree/evaltest, which I wrote up as an exercise.

[Michael-Gallo]

I think the map approach is more readable because it separates the options from the general eval logic, someone adding a new option now only needs to be concerned with a piece of code that exists solely for options. Finding any given option is easier and removing any given option is a one line change and to add a new option you only need to be concerned with adjusting the map. I also think it makes easier to modify and understand what eval is doing since there is no need to scroll through a multi hundred line switch to get to the end instead of having a relatively small function that can fit on your screen at one time.

That said, you were able to pull a bigger line count reduction than I was so I'm happy either way.

[joelim-work]

I think your approach makes a tradeoff - it makes the high level definition of the options more readable (i.e. option name followed by actions), at the cost of more infrastructure code that is required to support such a pattern. This also explains why your approach doesn't have as large of a line reduction count.

The only other complaint I might have is that I can't immediately see what code is being called for a given option - I would have to jump to the definition of the cleanup function(s). But it is probably a minor issue since most IDEs should have some way of doing this.

Anyway I think both approaches are a significant improvement over the current implementation. I think it's probably best if I leave it to @gokcehan to decide.

Thanks once again for your changes, and also for your efforts to improve the code.

[gokcehan]

I think both approaches are fine, but I think I'm slightly in favor of @joelim-work 's approach. I see the main benefit of such a cleanup is the refactoring of the option processing logic. I don't think long switch statements are problems in general. @joelim-work 's approach also have the advantage of combining three boolean states into a single case in a cleaner way in my opinion. Regarding the option names in complete.go, I thought about using the reflect on the gOpts struct at some point, but I did not have the chance to give it a try. We already use a similar logic in getOptsMap so I feel like it should be possible, but I'm not 100% sure. It may be a good idea to get rid of the sortType struct before as discussed elsewhere.

[joelim-work]

@gokcehan I agree with keeping things simple and replacing the sortType struct with individual variables for each sort-related option. I have submitted #1577 for this. I think it might be better to work on that first before attempting any more refactoring.

Regarding complete.go, I didn't consider the idea of using reflect on the gOpts struct, but it sounds like a good idea and in theory it should work. Something like this:

var gOptWords []string

func init() {
	t := reflect.TypeOf(gOpts)
	for i := 0; i < t.NumField(); i++ {
		field := t.Field(i)
		switch field.Type.Kind() {
		case reflect.Map:
			continue
		case reflect.Bool:
			name := field.Name
			gOptWords = append(gOptWords, name, "no"+name, name+"!")
		default:
			gOptWords = append(gOptWords, field.Name)
		}
	}
}

Also, I think the code for setlocal can be refactored since it has similar repeating logic. The only hard part is that for setlocal, there is extra logic where toggling a non-existent setting falls back on the global value. It should be possible, it's just more work and I'm not 100% sure if there is a use case for this (i.e. setlocal <path> option!.