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

MELPA recipe for DDSKK to GNU Emacs #2116

Closed
wants to merge 1 commit into from
Closed

MELPA recipe for DDSKK to GNU Emacs #2116

wants to merge 1 commit into from

Conversation

yoshida-mediba
Copy link
Contributor

@purcell
Copy link
Member

purcell commented Oct 27, 2014

I'm concerned that this is bundling several third-party files (eg. cdb.el) together into a new repository. Those files should probably be separate packages.

Are you the maintainer of this code? It's not clear how the CVS repo in the recipe is connected to the author.

In fact, the main skk.el file has some problems, and can't be correctly parsed by package-buffer-info, so it's not yet ready for packaging by MELPA.

Additionally, code like the following is bad practice, because it unconditionally executes the code:

;;;###autoload
(add-hook 'after-init-hook
      (lambda ()
        (when (and (symbol-value 'init-file-user)
               skk-preload)
          (skk-preload)))
      t)

The user should make this choice, because installing a package does not imply that the user wants to enable it.

@purcell purcell added recipes awaiting-upstream Awaiting action from an upstream maintainer labels Nov 1, 2014
@tshinohara tshinohara mentioned this pull request Dec 1, 2014
@myuhe
Copy link
Contributor

myuhe commented Dec 21, 2014

This fixed #2214 and upstream changes.
We can close this.

@purcell purcell closed this Dec 21, 2014
@tetsuotsukamoto
Copy link

Hi Steve,

Thank you for your concern but I think you misunderstood the above code added to after-init-hook. By reading the code carefully you would know that it doesn't enable the package automatically. The function skk-preload is called in order to preload the package at startup but ONLY IF the user option skk-preload is explicitly set to non-nil by the user as it is nil by default. Therefore the code actually gives the user the choice, contrary to your guess. You are wrong if you think that the code takes away the user's right to decide. In terms of good/bad practice the code shouldn't need any change. The code should be kind to the user. Please advise with a good reason if the code still looks like a bad practice to you. Otherwise please update your understanding of the code and accept the original code.

Thanks
Tetsuo

@purcell
Copy link
Member

purcell commented Jan 13, 2015

@tetsuotsukamoto Yes, you're right: I should have read more carefully. Thanks for your patience!

@tetsuotsukamoto
Copy link

Thanks for your understanding!

On 13 Jan 2015, at 11:24 pm, Steve Purcell notifications@github.com wrote:

@tetsuotsukamoto https://github.com/tetsuotsukamoto Yes, you're right: I should have read more carefully. Thanks for your patience!


Reply to this email directly or view it on GitHub #2116 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-upstream Awaiting action from an upstream maintainer recipes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants