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 ruby-factory-mode recipe #3190

Merged
merged 1 commit into from Oct 11, 2015
Merged

Add ruby-factory-mode recipe #3190

merged 1 commit into from Oct 11, 2015

Conversation

sshaw
Copy link
Contributor

@sshaw sshaw commented Oct 9, 2015

Minor mode for Ruby test object generation libraries

https://github.com/sshaw/ruby-factory-mode

I'm the maintainer.

@milkypostman
Copy link
Member

Couple small things needed before merge.,

  • remove the autoload which adds the mode to ruby-mode-hook. Standard practice is to provide docs on how to enable the mode because we don't want users to have unexpected behavior just because they installed a package.
  • change :files spec to be (:defaults "snippets") which is more future proof and will pick up the el file.
  • How are you using the inflections package? It doesn't seem like it's being used, but I don't know a lot about that package and what the functions are that it provides.

@purcell any issue with https://github.com/sshaw/ruby-factory-mode/blob/master/ruby-factory-mode.el#L157 ? I'm not sure the best way to deal with this. I don't know how we've dealt with this in the past.

@sshaw
Copy link
Contributor Author

sshaw commented Oct 9, 2015

Thanks for the feedback.

How are you using the inflections package? It doesn't seem like it's being used

I'm using it to get the plural and singular forms of a file's name. Some examples:

https://github.com/sshaw/ruby-factory-mode/blob/master/ruby-factory-mode.el#L111
https://github.com/sshaw/ruby-factory-mode/blob/master/ruby-factory-mode.el#L104

@purcell any issue with https://github.com/sshaw/ruby-factory-mode/blob/master/ruby-factory-mode.el#L157

Here's are some examples of a MELPA package with snippets: #744, #3017

They more or less do the following:

;;;###autoload
(defun something-initialize ()
  (let ((snip-dir (expand-file-name "snippets" snippets-root)))
    (when (boundp 'yas-snippet-dirs)
      (add-to-list 'yas-snippet-dirs snip-dir t))
    (yas-load-directory snip-dir)))

;;;###autoload
(eval-after-load "yasnippet"
  '(something-initialize))

In my case (yas-activate-extra-mode mode) was also needed else expansion would not work for the given buffer.

@purcell
Copy link
Member

purcell commented Oct 9, 2015

Agree re. autoloading the ruby-mode-hook modification -- the user should be responsible for this.

Re. the snippet setup, just being able to (require 'yasnippet nil t) does not mean that the user wants yasnippet loaded. Better would be to test whether yasnippet is enabled, e.g. via something like (bound-and-true-p yasnippet-mode).

Further feedback:

  • You can't use "C-c f t" as a keybinding, because C-c LETTER is reserved for end-user customization
  • This is a minor mode, but using the -mode feature prefix is frowned upon except in the case of major modes for language support. So perhaps just call the library ruby-factory.

@sshaw
Copy link
Contributor Author

sshaw commented Oct 10, 2015

This is a minor mode, but using the -mode feature prefix is frowned upon except in the case of major modes for language support.

Since when is this the case? Every minor mode I've ever heard of ends in -mode. hs-minor-mode, linum-mode, abbrev-mode, ace-jump-mode, etc...

So perhaps just call the library ruby-factory.

If you saw ruby-factory, what would you think it does?

Removing the -mode removes the package's identity/purpose. ruby-factory and ruby-factory-mode convey different meanings. The former being a bit muddled: is it a video game? Is it a template system for building ruby projects? Does it have anything to do with programming?

@purcell
Copy link
Member

purcell commented Oct 10, 2015

Since when is this the case? Every minor mode I've ever heard of ends in -mode. hs-minor-mode, linum-mode, abbrev-mode, ace-jump-mode, etc...

Perhaps I wasn't clear enough. Yes, of course, the minor mode itself gets the -mode suffix, but the containing file/feature generally shouldn't.

Don't forget that all the symbols in a package should be prefixed with the full package name, which is another standard elisp convention, which also points to keeping the package name brief, since the ruby-factory- prefix is shorter than ruby-factory-mode-.

I'm not going to dig my heels in over this, but I wouldn't be doing my job if I didn't take the time to explain how things are supposed to work. :-)

@purcell
Copy link
Member

purcell commented Oct 10, 2015

P.S. Examples of popular packages which provide a minor mode yet do not contain -mode in their name are: yasnippet, company, paredit, aggressive-indent etc.

@sshaw
Copy link
Contributor Author

sshaw commented Oct 10, 2015

@purcell made some updates based on your feedback (thanks 👍). Checkout the melpa-fixes branch.

@purcell
Copy link
Member

purcell commented Oct 10, 2015

Looks good! Note that

(add-hook 'ruby-mode-hook
    (lambda () (ruby-factory-mode)))

is equivalent to

(add-hook 'ruby-mode-hook 'ruby-factory-mode)

@milkypostman
Copy link
Member

can you fix up that final add-hook change and merge back into master?

@sshaw
Copy link
Contributor Author

sshaw commented Oct 11, 2015

Just fixed it.

milkypostman added a commit that referenced this pull request Oct 11, 2015
Add ruby-factory-mode recipe
@milkypostman milkypostman merged commit 3ce422c into melpa:master Oct 11, 2015
@milkypostman
Copy link
Member

Thanks!

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