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

Add playground #5242

Merged
merged 3 commits into from Feb 4, 2018
Merged

Add playground #5242

merged 3 commits into from Feb 4, 2018

Conversation

akirak
Copy link
Contributor

@akirak akirak commented Jan 16, 2018

Brief summary of what the package does

Run Emacs on a sandboxed $HOME

Direct link to the package repository

https://github.com/akirak/emacs-playground

Your association with the package

Maintainer

Relevant communications with the upstream package maintainer

None needed

Checklist

Please confirm with x:

@alphapapa
Copy link
Contributor

alphapapa commented Jan 16, 2018

Hi,

Have you considered naming the package something like playground or sandbox? play is quite vague, and the first thing that came to mind for me was an audio utility (because there's a play utility in SoX).

By the way, while it's convenient, it's not a good idea to suggest that people use curl to install things, like:

curl https://raw.githubusercontent.com/akirak/play.el/master/play.el > ~/.emacs

It's a significant security risk. (Not that git or MELPA are necessarily secure, either, but in general, it's not a good practice and shouldn't be recommended.)

I haven't looked at your code in detail, but from a quick glance, it looks well done. One other note: I saw a TODO about adding Helm support, but you don't necessarily need to do that, because Helm can operate as a replacement for completing-read (and most users probably configure it that way, as it's the recommended way).

@akirak
Copy link
Contributor Author

akirak commented Jan 16, 2018

@alphapapa Thank you for your comments.

I don't mind changing the package name to playground or something, but I will have to change the names of all functions and variables (and even update the README), as their namespace is going to change from play- to playground-. Could you give me a little time until I send you a new PR?

Installing using curl is not a necessity. It's just an option, as this packages doesn't require any dependencies. I've understood your point, so I'll delete the instruction after this package is added to MELPA.

As for the Helm support, it would be better to provide a dedicated Helm function, because there are multiple (local and remote) sources for sandboxes. However, I won't work on it for now, as it will require additional dependency on Helm, which seems an overkill for this trivial stuff.

@alphapapa
Copy link
Contributor

alphapapa commented Jan 16, 2018

I don't mind changing the package name to playground or something, but I will have to change the names of all functions and variables (and even update the README), as their namespace is going to change from play- to playground-

Yeah, generally renaming an Emacs package is very easy, because doing a simple M-% play- RET playground- RET will take care of it. :)

Could you give me a little time until I send you a new PR?

I'm not MELPA staff, I'm just an observer who tries to help by giving feedback sometimes. In any case, it's your package, so you can take all the time you want.

As for the Helm support, it would be better to provide a dedicated Helm function, because there are multiple (local and remote) sources for sandboxes. However, I won't work on it for now, as it will require additional dependency on Helm, which seems an overkill for this trivial stuff.

I see, good point. Helm's multi-source support is indeed very useful.

@akirak akirak changed the title Add play Add playground Jan 17, 2018
@purcell
Copy link
Member

purcell commented Jan 22, 2018

Thanks! Quick notes:

  • Will this work on Windows? Isn't there a built-in function/variable you could use to determine the home directory?
  • (when (null repo) is equivalent to (unless repo.
  • This line will break horribly if the home directory has a space in it. Use shell-quote-argument whenever you build commands as strings.

Hope that helps!

@akirak
Copy link
Contributor Author

akirak commented Jan 22, 2018

@purcell Thank you for helpful comments.

Will this work on Windows? Isn't there a built-in function/variable you could use to determine the home directory?

Windows was not in mind at the time of writing this package, because I don't have a (real) Windows machine. However, your idea can help some people who would wish to use playground no Windows.

I can test Playground on Windows VM images and/or my 15-year-old (!) Windows XP license. It will take some time, but I don't mind working on it, given that it is a one-shot task. The problem is that there seem to be multiple variants of Emacs for Windows, and I don't know how they work. Could you elaborate me on how to get the original home directory (looked up by Emacs) on Windows?

This line will break horribly if the home directory has a space in it. Use shell-quote-argument whenever you build commands as strings.

I have never imagined a situation where a login name contains a space. I will fix this, but I will have to set up a new test, which can take some time.

@purcell
Copy link
Member

purcell commented Feb 3, 2018

Sent you akirak/emacs-playground#5.

@purcell purcell merged commit f062a74 into melpa:master Feb 4, 2018
@purcell
Copy link
Member

purcell commented Feb 4, 2018

Merged now - welcome to MELPA!

Ambrevar pushed a commit to Ambrevar/melpa that referenced this pull request Feb 13, 2018
* Add play

* Add playground

* Delete play
waymondo pushed a commit to waymondo/melpa that referenced this pull request Feb 20, 2018
* Add play

* Add playground

* Delete play
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.

None yet

3 participants