spy16 / sabre

Sabre is highly customisable, embeddable LISP engine for Go. :computer:
GNU General Public License v3.0
28 stars 5 forks source link

Refactor Value interface #16

Open spy16 opened 4 years ago

spy16 commented 4 years ago

Most of the Value types (except for Symbol, List, Vector, Set), return themselves on Eval().. It seems like a redundant requirement to have all Values implement the Eval() method when in most cases it's not required.. I am considering may be it would be simpler to remove this and make Eval() an optional interface.. We have 3 possibilities:

  1. Remove the Value type altogether and deal with interface{} types.
  2. Turn Value into a marker interface like below:

     // Remove Eval() from current Value interface and rename String() to Source()
     // to avoid ambiguity with commonly used fmt.Stringer
     type Value interface {
           Source() string 
     }
  3. Not do anything and keep it as is.

If we choose 1 or 2, eval requirement can become an optional interface:

type Expr interface {
      Eval(scope Scope) (Value, error) // or (interface{}, error) based on 1 or 2
}

@lthibault Any thoughts on this ?

lthibault commented 4 years ago

If we go for option 1, I think we should at least give the empty interface a type, e..g: type Value interface{}. This achieves two things:

  1. keeps things readable (avoids questions like "is this a Sabre interface{} or something else?)
  2. allows us to easily add methods to the interface later, if needed.

Note that there is precedent for this approach.

Concerning option 2, I'm not sure what we obtain from having the Source() method.

Option 3 is also fine. It avoids a type assertion, which has performance implications. I'm not sure how much we care about this, though.

Ultimately it's up to you -- I don't have strong feelings about this either way.

spy16 commented 4 years ago

Yep agreed on all. Performance is not big enough concern to warrant any targeted optimization but I would still like to keep the overhead to minimum. I've decided to keep it as is for now. We can think about this again at some point later..