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 demo-it package to recipes #1885

Closed
wants to merge 1 commit into from
Closed

Add demo-it package to recipes #1885

wants to merge 1 commit into from

Conversation

howardabrams
Copy link

A collection of utility functions for creating demonstrations and
presentations from within Emacs.

For details, see https://github.com/howardabrams/demo-it.git

A collection of utility functions for creating demonstrations and
presentations from within Emacs.

For details, see https://github.com/howardabrams/demo-it.git
@purcell
Copy link
Member

purcell commented Jul 31, 2014

Thanks - interesting stuff!

There are quite a few issues here which need to be addressed before we can add the package.

  • File isn't formatted as a package: it lacks the requisite header boilerplate, for a start. Try enabling auto-insert-mode, then create a fresh demo-it.el to see how that should look, and finally insert your code into that new conformant template.
  • The recipe as-is would bundle example.el into the package, which is undesirable. Consider putting example.el in an examples subdir to solve this. Otherwise, you'd have to list only demo-it.el in the recipe.
  • The mix of demo-it/ and demo-it- identifier prefixes serves no obvious purpose and is unidiomatic. It's conventional to occasionally use / or -- to denote package-internal identifiers, but that doesn't seem to be what you're doing here. I suggest you just use the conventional demo-it- prefix style throughout.
  • You shouldn't just global-set-key bindings at the top level. Either provide the user with a function which will initialize the bindings at a time of his choosing, or define a quick global mode which has the bindings in its mode map: this would be the cleanest option.
  • You should avoid defining unprefixed identifiers, e.g. autofeaturep, hide-mode-line etc.
  • autofeaturep itself uses an unreliable approach. Use (eval-after-load 'expand-region (progn ...your code here...)) instead.
  • defvar-local is not available in Emacs < 24. I strongly suggest you just use defvar combined with make-variable-buffer-local in order to maximise compatibility.

Hope that helps!

-Steve

@howardabrams
Copy link
Author

Thanks for the feedback! I'll get back to you on those.

@howardabrams
Copy link
Author

Alright, I think I have cleaned up the project significantly. @purcell, do you notice anything else I should address?

Thanks again for the feedback.

@purcell
Copy link
Member

purcell commented Aug 19, 2014

Nope, looks great!

@purcell purcell closed this in 1dec587 Aug 19, 2014
@purcell
Copy link
Member

purcell commented Aug 19, 2014

I've committed a slightly different recipe, using the github fetcher shorthand.

@howardabrams
Copy link
Author

Sounds just fine. Thanks, again!

@purcell
Copy link
Member

purcell commented Aug 19, 2014

Package page is here, BTW: http://melpa.milkbox.net/#/demo-it

vspinu pushed a commit to vspinu/melpa that referenced this pull request Nov 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants