Add skinny-samurai-theme

[yasuyk]

https://github.com/deftpunk/skinnySamurai

/cc @deftpunk

[purcell]

The hacky skinny-samurai-numbers-face should be removed: a theme has no business altering the font-lock-keywords of other modes. Plus, according to the comments, it doesn't work anyway.

@deftpunk The correct way to add a face to those modes would be to write a "highlight-numbers" mode, and then your theme could provide a customisation for the highlight-numbers face.

[purcell]

Additionally, the code should not be modifying safe-local-eval-forms globally:

;;;###autoload
(add-to-list 'safe-local-eval-forms
             '(when (require 'rainbow-mode nil t) (rainbow-mode 1)))

And finally, the following block is wrong for 2 reasons:

(if window-system
    (progn
      (set-background-color "#ffffd7")
      (set-foreground-color "#5f5f5f")))

Firstly, you shouldn't globally set the background color. Just modify the default face in the theme.

And secondly, window-system can have a different value in each frame, and this code will not be executed every time the theme is loaded: see http://stackoverflow.com/a/5801740/247020

[milkypostman]

I think we already have a package to highlight numbers in MELPA. It has the cryptic title of number-font-lock-mode

[deftpunk]

Okay ... the law of unintended consequences strikes again; I assumed this little experiment with color themes was in a dark enough corner not to be noticed. Certainly was not intended to show up on MELPA and really don't want it to.

@yasuyk: I have made changes per the comments. Plus pull those changes into your fork and submit your fork to MELPA. Let me know when you have done so, I will then take my repo off of github and put it somewhere private.

@purcell: Writing a mode to highlight numbers in your buffer, instead of just creating another face, seems highly inefficient and is definitely counter-intuitive. "(un)like what many editors provide by default" Seeing its already been done makes the point mute, will remove.

I cribbed the safe-local-eval-forms code to get rainbow-mode working from the following: bbatsov/zenburn-emacs#107
incorrect??

Will remove the window-system block, should have surmised that it works that way, makes sense when you think about it for a bit.

@milkypostman : thank you for pointing out number-font-lock-mode, assuming that you are not being sarcastic. Thank you for MELPA irregardless.

[yasuyk]

@deftpunk I apologize for troubling you. I don't intend to add this package against your will. You don't need to change repository to private. If you changed your mind in the future, You may send PR for recipe again.

@milkypostman @purcell I am going to close this PR for now, right?

[milkypostman]

Yeah I was just joking.

I think what the mode does is just create a face or at least that is what
it should do. I sent an email a long time ago pleading with emacs
developers to add this as a face to the default emacs but they wouldn't.

I hope you would reconsider providing your theme to melpa as I love theme
choices. Our method of vetting packages is simply to try and improve the
quality of packages for everyone use and try to find problems that could
crop up for users. The cautions about buffer local variables and number
don't lock are things that can confuse some users.

Best, Donald
On Feb 8, 2014 11:29 AM, "deftpunk" notifications@github.com wrote:

Okay ... the law of unintended consequences strikes again; I assumed this
little experiment with color themes was in a dark enough corner not to be
noticed. Certainly was not intended to show up on MELPA and really don't
want it to.

@yasuyk https://github.com/yasuyk: I have made changes per the
comments. Plus pull those changes into your fork and submit your fork to
MELPA. Let me know when you have done so, I will then take my repo off of
github and put it somewhere private.

@purcell https://github.com/purcell: Writing a mode to highlight
numbers in your buffer, instead of just creating another face, seems highly
inefficient and is definitely counter-intuitive. "(un)like what many
editors provide by default" Seeing its already been done makes the point
mute, will remove.

I cribbed the safe-local-eval-forms code to get rainbow-mode working from
the following: bbatsov/zenburn-emacs#107bbatsov/zenburn-emacs#107
incorrect??

Will remove the window-system block, should have surmised that it works
that way, makes sense when you think about it for a bit.

@milkypostman https://github.com/milkypostman : thank you for pointing
out number-font-lock-mode, assuming that you are not being sarcastic. Thank
you for MELPA irregardless.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1472#issuecomment-34553339
.

[purcell]

I cribbed the safe-local-eval-forms code to get rainbow-mode working from the following: bbatsov/zenburn-emacs#107
incorrect??

I'd argue that yes, @bbatsov should also not be deciding for the user what forms are safe for local evaluation. It's really sidestepping a check that is there for the user's protection, and it's not right for a theme to unilaterally declare that rainbow-mode is safe.

[bbatsov]

Pretty sure the code in question was reverted shortly after it was merged.

[Fanael]

@milkypostman: are you being sarcastic, or do you really think the name is cryptic? In the latter case, I'll gladly rename it if you have a better idea for a name.

[purcell]

@Fanael Many end-users of Emacs actually won't be familiar with the term "font-lock", since it's part of the internal elisp API, so that might be why @milkypostman felt it was a little cryptic.

I might personally have picked "highlight-numbers" or something similar, but there's really no need to rename it unless you want to.

[milkypostman]

@Fanael I was making a joke that the name is well-named but is not easy to remember. that is all. it was hard to understand as a joke but I was feeling punchy.

[milkypostman]

@Fanael @purcell is correct that font-lock is a bit obscure but I think that's a downfall of emacs devs, not you. I would leave it as is!

[purcell]

Hey @deftpunk -- shall we go ahead and add this theme to MELPA now you've fixed it up, or would you still prefer that we omit it?

[purcell]

Closing -- please re-open this PR if it becomes viable in the future.