Skip to content
This repository has been archived by the owner on May 23, 2021. It is now read-only.

Refactor Value interface #16

Open
spy16 opened this issue Feb 26, 2020 · 2 comments
Open

Refactor Value interface #16

spy16 opened this issue Feb 26, 2020 · 2 comments

Comments

@spy16
Copy link
Owner

spy16 commented Feb 26, 2020

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
Copy link
Collaborator

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
Copy link
Owner Author

spy16 commented Feb 27, 2020

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..

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants