spy16 / sabre

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

Slang #6

Closed spy16 closed 4 years ago

spy16 commented 4 years ago

Slang (short for Sabre Lang) - A tiny LISP dialect implemented using Sabre.

spy16 commented 4 years ago

@lthibault Any thoughts on this? I was thinking a package slang can have a simple usable language implementation with REPL, name-spaced scope using Sabre . This package can act as a reference usage.

lthibault commented 4 years ago

Ah, wonderful! This is a huge help in understanding how the pieces fit together 👍

I'm going integrate some of the design of Slang into my own project and then post it to github. If you're interested, it might provide some useful insight into how I'm using the library.

spy16 commented 4 years ago

@lthibault take a look at https://github.com/spy16/sabre/pull/6/commits/679889252ed6020ac704dc76265dfb85c4f6763c. It implements our discussion. I went with repl.Loop(ctx) function. Any thoughts on that (i.e., Run(ctx) vs Loop(ctx))?

spy16 commented 4 years ago

While repl has turned out be a nice little package, i think the Runtime interface adds a bit of unnecessary complexity. For building a custom language, all that is really necessary with Sabre is to bind custom functions into a Scope implementation and pass this scope + forms to sabre.Eval. Historically also, LISP had concept of Environment against which data structures/forms/s-expressions are evaluated. I am thinking may be it would be good enough to have repl.New(env sabre.Scope, opts ...Option) as the actual signature. Scope can implement an optional interface to support namespaces.

type NamespacedScope struct {
        SwitchNS(ns string)
        CurrentNS() string
}

This would make the REPL usage much simpler since user doesn't really have to manually implement any interfaces.. repl.New(sabre.NewScope(nil), ....) would work for most cases.

lthibault commented 4 years ago

+1 for eschewing repl.Runtime in favor of sabre.Scope. Also agree with the optional interface approach to providing namespaces.

spy16 commented 4 years ago

@lthibault I have changed inline function to ErrMapper type and also added WithReaderFactory to allow customizing reader initialization. Let me know what you think..

Also, currently, binary is available as sabre (because of cmd/sabre/main.go). But given that the binary actually running slang as the runtime, do you think the binary should be available as slang (i.e., cmd/slang/main.go) ?

lthibault commented 4 years ago

Do you think the binary should be available as slang?

Yes, I think that's a good idea. I initially started a cmd/slang package because I hadn't noticed that cmd/sabre was doing what I wanted ... so I'm sure that will confuse others, too!

Reviewing changes now.

spy16 commented 4 years ago

All fixed. I think it is good time to merge this to master and continue from there.

lthibault commented 4 years ago

@spy16 can we increment the version tag too?

spy16 commented 4 years ago

Yes will do that. v0.2.0 ? (Still not mature enough to be v1 i think. But big changes so minor release)

lthibault commented 4 years ago

Yes will do that. v0.2.0 ? (Still not mature enough to be v1 i think. But big changes so minor release)

Yes, agreed on all counts.

lthibault commented 4 years ago

BTW, I think this is ready to merge ... just want to make sure you weren't waiting on me! 😄

spy16 commented 4 years ago

Ah thanks. But it was late night for me and decided to do it later. Will do it sometime today.

spy16 commented 4 years ago

I want to cleanup the commits here a little bit before merging since there are lot of unwanted/experimental commits. Any ideas? Interactive re-base is getting lot of conflicts for some reason.

One other option is to squash and merge, but that will lose author information unless we add Co-authored-by

lthibault commented 4 years ago

I want to cleanup the commits here a little bit before merging since there are lot of unwanted/experimental commits. Any ideas? Interactive re-base is getting lot of conflicts for some reason.

One other option is to squash and merge, but that will lose author information unless we add Co-authored-by

Sorry, I was a bit slow to respond.

I'm generally a fan of rebasing into the master branch to the extent possible. If this situation ever comes up again, let's just discuss again at that point (and hope it doesn't happen to often!)

In any case, I very much appreciate your consideration of attribution. Thanks for that 😃

(P.S.: first collaborative PR closed! Woohoo! 🎉)

spy16 commented 4 years ago

Yea i like to rebase onto master as well. But before that i also usually cleanup commits a bit by squashing some of them to make each commit a sensible and usable set of changes.

For this PR, i have included the Co-authored-by section which shows up in this merge commit. But unfortunately it doesn't show in repository contributor list.. :(

lthibault commented 4 years ago

No worries. There will be a next time 😃