Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More convenient REPL support #31

Merged
merged 4 commits into from
Jun 16, 2019
Merged

Conversation

oubiwann
Copy link
Contributor

I've tweaked the REPL support to include some sane defaults. Added custom REPL tweaking to its own Markdown file in the docs dir.

@oubiwann oubiwann force-pushed the convenient-repl-support branch from 174f1ee to 3d3f84f Compare June 13, 2019 05:18
@swadey
Copy link
Owner

swadey commented Jun 13, 2019

hey, thanks for this. i think we need to discuss collectively whether a repl should be included or if this set of components makes more sense in the corresponding LispREPL package. I don't think we fully resolved this.

src/repl.jl Outdated
prompt_color = :yellow,
start_key = ")",
mode_name = "Lisp Mode")
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would instead do this:

function init_repl(; prompt_text="jλ> ", prompt_color = :red, start_key = ")", sticky=true)
	    ReplMaker.initrepl(lisp_eval_helper,
	                  repl = Base.active_repl,
	                  valid_input_checker = lisp_reader,
	                  prompt_text = prompt_text,
	                  prompt_color = prompt_color,
	                  start_key = start_key,
                          sticky_mode=sticky,
	                  mode_name = "Lisp Mode")
end

so that the user may choose the prompt text, colour, key and whether or not the mode is sticky at startup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice; haven't done Julia since 2015 or so -- love the feedback!

@MasonProtter
Copy link
Contributor

i think we need to discuss collectively whether a repl should be included or if this set of components makes more sense in the corresponding LispREPL package. I don't think we fully resolved this.

Personally, I don't really see why one wouldn't want it to be included but of course this is your repo so its your call.

@oubiwann
Copy link
Contributor Author

I actually did start this change with the REPL code as its own package, but then I second-guessed myself, thinking I was over complicating it and that the PR would be 👎 'ed ;-)

(Because, based on other Lisps like Clojure, the next step was to actually have the LispReader.jl package ... even though it would be tiny; readers are typically useful in more than just the REPL context.)

Then, as I was thinking about pulling it into the LispSyntax.jl, I thought about how this package is really more than just syntax, the project seems like it might be growing beyond the name ... and in that sense, it felt right just being here ...

Now you've had a peek inside me brains!

@oubiwann
Copy link
Contributor Author

Btw: no strong opinion here! Totally cool with any direction.

@MasonProtter
Copy link
Contributor

Alternatively, if we want to keep the scope of this repository small, things like #29 and #28 (if I ever get around to them) and this PR could all go in separate packages and then there can just be a frontend package that pulls them all into one namespace and exports it.

@c42f
Copy link
Collaborator

c42f commented Jun 15, 2019

Regarding the parser side of things, there's been some discussion of having a SExpressions parser as a stdlib to support testing of the julia compiler lowering pass. See JuliaLang/julia#32201 and TotalVerb/SExpressions.jl#3 (comment)

Personally I'm somewhat biased toward scheme rather than clojure syntax for quoting etc but I imagine we should be able to support both from the same parser.

@oubiwann
Copy link
Contributor Author

Thanks again, @MasonProtter: I've pushed the changes you suggested.

@swadey
Copy link
Owner

swadey commented Jun 16, 2019

Given all of the comments, it might be better to refactor the s-expression parser at some point. For now, I'm going to merge this. Eventually moving out some of the functionality into submodules as Mason suggests would be good.

@swadey swadey merged commit f32d87f into swadey:master Jun 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants