-
Notifications
You must be signed in to change notification settings - Fork 570
Add back .purs-repl file support #2735
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
Conversation
Does this allow you to put basically anything inside .psci? Not that I feel particularly strongly that we should be restricting it, but it might be worth discussing. |
Yes it does right now. I think it might be better to limit it to imports only. There is also a security risk associated with running arbitrary code on behalf of the user. |
Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I'll let you decide what you want to do about my comment.
app/Command/REPL.hs
Outdated
case parseCommand l of | ||
Left err -> liftIO (putStrLn err >> exitFailure) | ||
Right cmd@Import{} -> handleCommand' state cmd | ||
Right _ -> liftIO (putStrLn "Only import declarations are supported in the .psci file" >> exitFailure) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nicer to print a warning, ignore declarations which aren't imports, and continue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
👍 |
|
Yeah, that makes sense. It is called PSCi elsewhere though. |
* Add back .psci file support * Restrict .psci to import statements * Only warn * Use .purs-repl
* Add back .psci file support * Restrict .psci to import statements * Only warn * Use .purs-repl
Fixes #2734
It looks like this:
cc @chexxor