Allow hyphenated names for `rails plugin new` generator #13601

Merged
merged 1 commit into from Jan 30, 2015

Projects

None yet

10 participants

@robinboening
Contributor

It was not possible to create a new gem with a hyphenated name via the rails plugin new scaffold generator.

In reference to http://guides.rubygems.org/name-your-gem/ hyphens should be used for gems that extend other gems.

Also added two missing tests for the naming of rails plugins via the rails plugin new in a separate commit.

@egilburg egilburg and 1 other commented on an outdated diff Jan 5, 2014
...lib/rails/generators/rails/plugin/plugin_generator.rb
@@ -255,6 +255,15 @@ def name
end
end
+ def namespaced_name
@egilburg
egilburg Jan 5, 2014 Contributor

.gsub without bang already returns new object without nils.

def namespaced_name
  @namespaced_name ||= name.gsub('-', '/')
end
@robinboening
robinboening Jan 6, 2014 Contributor

Good point. Fixed it and updated the pull request. Thanks.

@dmathieu dmathieu and 1 other commented on an outdated diff Jan 6, 2014
railties/test/generators/plugin_generator_test.rb
content = capture(:stderr){ run_generator [File.join(destination_root, "43things")] }
assert_equal "Invalid plugin name 43things. Please give a name which does not start with numbers.\n", content
+
+ content = capture(:stderr){ run_generator [File.join(destination_root, "plugin")] }
+ assert_equal "Invalid plugin name plugin. Please give a name which does not match one of the reserved rails words.\n", content
+
+ content = capture(:stderr){ run_generator [File.join(destination_root, "Digest")] }
+ assert_equal "Invalid plugin name Digest, constant Digest is already in use. Please choose another plugin name.\n", content
@dmathieu
dmathieu Jan 6, 2014 Contributor

Why are you adding these two tests?

@robinboening
robinboening Jan 6, 2014 Contributor

I want to make sure that all the files gets nested correctly.

EDIT: Sorry, I thought you were talking about the other two tests, for the folder nesting... So my first sentence does not make sense. I added this tests in a separate commit, just because I saw they should be written.

@dmathieu
dmathieu Jan 6, 2014 Contributor

What I mean is that it's not in the scope of the hyphenated names PR.
It makes blaming the file for these lines harder, and I don't really see the value added with them.

@robinboening
robinboening Jan 6, 2014 Contributor

I moved that commit out of this pull request. and opend a separate one: #13611

@egilburg egilburg and 1 other commented on an outdated diff Jan 6, 2014
...lib/rails/generators/rails/plugin/plugin_generator.rb
@@ -308,8 +312,8 @@ def camelized
end
def valid_const?
- if original_name =~ /[^0-9a-zA-Z_]+/
- raise Error, "Invalid plugin name #{original_name}. Please give a name which use only alphabetic or numeric or \"_\" characters."
+ if original_name =~ /[^0-9a-zA-Z_-]+/
@egilburg
egilburg Jan 6, 2014 Contributor

This regex can now be replaced with /[^[:word:]]+/ or just /[^\w]+/, per http://www.regular-expressions.info/posixbrackets.html

edit: fixed example

@robinboening
robinboening Jan 6, 2014 Contributor

I think it could be replaced before these changes.
[:word:] means letters, numbers and underscores. (But not dashes)
Right?

@egilburg
egilburg Jan 6, 2014 Contributor

or even simpler: /\W+/

:-)

@egilburg
egilburg Jan 6, 2014 Contributor

according to posix document, word includes dashes

edit: nvm i was wrong :(

@senny
Member
senny commented Jan 6, 2014

@robinboening Does your solution address #4737. The hyphen check was added with #4737 to solve that issue.

@robinboening
Contributor

@senny If I didnt read wrong, the issue in #4737 meant: When creating a rails plugin with a name like 'some-3.1-plugin_name' the plugin generator created a folder named some_3.1_plugin_name to load the application_helper.rb from - which raised a LoadError.

That issue was fixed by not allowing "." and "-" in the plugin generator anymore.

The changes in my pull request will now allow "-" in plugin names, but it differs to the old behavior: It will not just create one folder with the plugin name anymore. It will nest the files (for example application_helper.rb) into folders split by the dashes.

Examples

The name some-3.1-plugin_name would still be rejected, because dots are not allowed.

A name like some-31-plugin_name would be be ok. The created folders will then look like: app/helpers/some/31/plugin_name/application_helper.rb. This will not raise an error.

I created a rails plugin with my changes and run rake after that without any errors:

$ cd some-31-plugin_name
$ rake
Run options: --seed 31847
# Running:
.
Finished in 0.029617s, 33.7644 runs/s, 33.7644 assertions/s.
1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

EDIT: Sorry I just saw its not correct what I said. The application_helper.rb and also the controller is still created in a folder named some-31-plugin_name. It does not raise an error, but I will change that.

EDIT 2: application_helper.rb and application_controller.rb are now nested into the namespace folders.

@egilburg egilburg and 1 other commented on an outdated diff Jan 6, 2014
railties/test/generators/plugin_generator_test.rb
@@ -213,6 +216,22 @@ def test_creating_engine_in_full_mode
assert_file "lib/bukkits.rb", /require "bukkits\/engine"/
end
+ def test_creating_engine_with_hyphenated_name_in_full_mode
+ run_generator [File.join(destination_root, "hyphenated-name"), "--full"]
+ assert_file "hyphenated-name/app/assets/javascripts/hyphenated/name"
+ assert_file "hyphenated-name/app/assets/stylesheets/hyphenated/name"
+ assert_file "hyphenated-name/app/assets/images/hyphenated/name"
+ assert_file "hyphenated-name/app/models"
+ assert_file "hyphenated-name/app/controllers"
+ assert_file "hyphenated-name/app/views"
+ assert_file "hyphenated-name/app/helpers"
+ assert_file "hyphenated-name/app/mailers"
+ assert_file "hyphenated-name/bin/rails"
+ assert_file "hyphenated-name/config/routes.rb", /Rails.application.routes.draw do/
+ assert_file "hyphenated-name/lib/hyphenated/name/engine.rb", /module HyphenatedName\n class Engine < ::Rails::Engine\n end\nend/
@egilburg
egilburg Jan 6, 2014 Contributor

If you are nesting the generated file, should the module name be nested as well?

module Hyphenated
  module Name
    class Engine < :: Rails::Engine
      # ...
    end
  end
end

This does increase scope of PR and requires new logic (e.g. ensuring that both Hyphenated and Name are valid, non-taken names, and that there is no confusion with the hyphenated-name prefix of the gem name itself. But without this, what is the real use case of wanting/needing to break the engine path into subfolders?

@robinboening
robinboening Jan 6, 2014 Contributor

You are right. Thats exactly what I saw a few hours ago.
The scope of that PR really increases, thats true. But imo its important to allow hyphens because its the sign for the namespaces. I think that naming convention should be pushed more.
I need some time to investigate what and how we can get to the goal.

@robinboening
Contributor

Right now, I added failing specs that test the module nesting in the files.

Btw: Is there a better way than force pushing later changes? (I prefer to just keep one commit - or will you squash multiple commits together when merging?)

@cappadona

Any updates on this? It's still not possible to generate a plugin with a hyphenated name using the generator in 4.1.6.

@josh-m-sharpe
Contributor

I pulled this an attemped to use it. 'rails plugin new foo-bar --mountable' resulted in modules named FooBar instead of the intended Foo::Bar.

The directory structure was correct though. Everything was nested inside "foo/bar"

However, after renaming all the classes from FooBar to Foo::Bar, I got this error attempting to create a controller - the error is because it's looking for engine.rb in "foo-bar" and not in "foo/bar"

rails generate controller Foo::Bar::Workers
/Users/jsharpe/.rvm/rubies/ruby-2.1.1/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:126:in require': cannot load such file -- /Users/jsharpe/foo-bar/lib/foo-bar/engine (LoadError) from /Users/jsharpe/.rvm/rubies/ruby-2.1.1/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:126:inrequire'
from /Users/jsharpe/.rvm/gems/ruby-2.1.1/gems/railties-4.1.8/lib/rails/engine/commands.rb:11:in <top (required)>' from /Users/jsharpe/.rvm/rubies/ruby-2.1.1/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:73:inrequire'
from /Users/jsharpe/.rvm/rubies/ruby-2.1.1/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:73:in `require'

@robinboening
Contributor

@cappadona I had the chance to take another look into the code and force pushed some implementation changes. Locally all tests are passing.

@crankharder thanks you were right: I forgot to take care of the bin/rails template where ENGINE_PATH gets defined. This is done and also tested now. Also the module names are now separated correctly. You could try again if you like.

@rafaelfranca
Member

@chancancode since you were talking about this today you could review this PR 😄 :trollface:

@robinboening
Contributor

Any news on this? ✌️

@chancancode
Member

Sorry for holding it up, I've been quite busy lately, but I'll try to review it next week. :(

👍 on the idea, I wanted that just very recently. Just have the check the implementation before merging :)

@robinboening
Contributor

Sure, thank you for your time. Have a nice weekend! :)

@chancancode chancancode and 1 other commented on an outdated diff Jan 28, 2015
...lib/rails/generators/rails/plugin/plugin_generator.rb
@@ -50,10 +50,10 @@ def gitignore
end
def lib
- template "lib/%name%.rb"
- template "lib/tasks/%name%_tasks.rake"
- template "lib/%name%/version.rb"
- template "lib/%name%/engine.rb" if engine?
+ template "lib/%underscored_name%.rb"
@chancancode
chancancode Jan 28, 2015 Member

Shouldn't this be lib/%namespaced_name%.rb?

If you generate an engine called "foo-bar", you should require foo/bar, not require foo_bar, no?

@robinboening
robinboening Jan 30, 2015 Contributor

Check

@chancancode chancancode and 1 other commented on an outdated diff Jan 28, 2015
...lib/rails/generators/rails/plugin/plugin_generator.rb
@@ -50,10 +50,10 @@ def gitignore
end
def lib
- template "lib/%name%.rb"
- template "lib/tasks/%name%_tasks.rake"
- template "lib/%name%/version.rb"
- template "lib/%name%/engine.rb" if engine?
+ template "lib/%underscored_name%.rb"
+ template "lib/tasks/%namespaced_name%_tasks.rake"
@robinboening
robinboening Jan 30, 2015 Contributor

This is already correct, isn't it? foo-bar will generate: lib/tasks/foo/bar_tasks.rake

@chancancode chancancode commented on the diff Jan 28, 2015
...ugin/templates/lib/tasks/%namespaced_name%_tasks.rake
@@ -1,4 +1,4 @@
# desc "Explaining what the task does"
-# task :<%= name %> do
+# task :<%= underscored_name %> do
@chancancode
chancancode Jan 28, 2015 Member

Not sure what's the "right" thing for this, perhaps it should be rake-namespace'd too? (However, this is commented out, so perhaps it's not a big deal.

@robinboening
robinboening Jan 30, 2015 Contributor

mhh, you are right, namespaced would be really consequent. But imho I think too much of extra code is needed to just throw in comments that 100% reflect how it might/should be used. Now it shows a usage example which is not that 100% pretty but also correct, right?

What do you think?

@chancancode chancancode and 1 other commented on an outdated diff Jan 28, 2015
...enerators/rails/plugin/templates/rails/application.rb
@@ -13,6 +13,6 @@
<% end -%>
Bundler.require(*Rails.groups)
-require "<%= name %>"
+require "<%= underscored_name %>"
@chancancode
chancancode Jan 28, 2015 Member

see above (I think this should be namespaced_name)

@robinboening
robinboening Jan 30, 2015 Contributor

Check

@chancancode chancancode and 1 other commented on an outdated diff Jan 28, 2015
railties/test/generators/plugin_generator_test.rb
@@ -44,6 +44,21 @@ def test_invalid_plugin_name_raises_an_error
assert_equal "Invalid plugin name Digest, constant Digest is already in use. Please choose another plugin name.\n", content
end
+
+ def test_valid_plugin_name_not_raising_an_error
+ content = capture(:stderr){ run_generator [File.join(destination_root, "my_plugin-extension")] }
+ assert_equal "", content
+
+ content = capture(:stderr){ run_generator [File.join(destination_root, "my_plugin-extension31")] }
+ assert_equal "", content
+ end
+
+ def test_hyphenated_plugin_name_underscores_filename_in_lib_folder
+ run_generator [File.join(destination_root, "hyphenated-name")]
+ assert_no_file "hyphenated-name/lib/hyphenated-name.rb"
+ assert_file "hyphenated-name/lib/hyphenated_name.rb", /module Hyphenated\n module Name\n # Your code goes here...\n end\nend/
@chancancode
chancancode Jan 28, 2015 Member

lib/hyphenated/name.rb (see above)

@robinboening
robinboening Jan 30, 2015 Contributor

Check

@chancancode chancancode and 1 other commented on an outdated diff Jan 28, 2015
railties/test/generators/plugin_generator_test.rb
@@ -44,6 +44,21 @@ def test_invalid_plugin_name_raises_an_error
assert_equal "Invalid plugin name Digest, constant Digest is already in use. Please choose another plugin name.\n", content
end
+
+ def test_valid_plugin_name_not_raising_an_error
+ content = capture(:stderr){ run_generator [File.join(destination_root, "my_plugin-extension")] }
+ assert_equal "", content
@chancancode
chancancode Jan 28, 2015 Member

In general, we usually avoid assert_it_doesnt_blow_uptests. Is there something more meaningful that we can assert on, or is this already covered by the tests below (and so it can be removed?)

@robinboening
robinboening Jan 30, 2015 Contributor

Alright, I will remove these lines and add some other assertions to cover mixed names with underscores and dashes.

@chancancode
Member

Sorry for the delay. This is looking good. Added a few questions.

Is there a better way than force pushing later changes? (I prefer to just keep one commit - or will you squash multiple commits together when merging?)

Either is fine.

Also, is this supposed to work with deeply nested modules? Something like foo-bar-baz that generates Foo::Bar::Baz::Engine. Seems like the code would be able to handle it just fine, but we should probably have a test for it.

@robinboening robinboening commented on an outdated diff Jan 30, 2015
railties/test/generators/plugin_generator_test.rb
@@ -252,6 +284,25 @@ def test_create_mountable_application_with_mountable_option
end
end
+ def test_create_mountable_application_with_mountable_option_and_hypenated_name
+ run_generator [File.join(destination_root, "hyphenated-name"), "--mountable"]
+ assert_file "hyphenated-name/app/assets/javascripts/hyphenated/name"
+ assert_file "hyphenated-name/app/assets/stylesheets/hyphenated/name"
+ assert_file "hyphenated-name/app/assets/images/hyphenated/name"
+ assert_file "hyphenated-name/config/routes.rb", /Hyphenated::Name::Engine.routes.draw do/
+ assert_file "hyphenated-name/lib/hyphenated/name/version.rb", /module Hyphenated\n module Name\n VERSION = "0.0.1"\n end\nend/
+ assert_file "hyphenated-name/lib/hyphenated/name/engine.rb", /module Hyphenated\n module Name\n class Engine < ::Rails::Engine\n isolate_namespace Hyphenated\n end\n end\nend/
+ assert_file "hyphenated-name/lib/hyphenated_name.rb", /require "hyphenated\/name\/engine"/
@robinboening
robinboening Jan 30, 2015 Contributor

This also needs to be namespaced like this lib/hyphenated/name.rb

@robinboening robinboening Allow hyphenated names for `rails plugin new` generator.
It was not possible to create a new gem with a hyphenated name via the `rails plugin new` generator.

The naming guide of rubygems clearly says dashes should be used for gems that extend other gems. http://guides.rubygems.org/name-your-gem/
ee9e4c3
@robinboening
Contributor

@chancancode I rebased with current master and (force) pushed the changes.

Deeply nested modules (like your example) are also working. I added a test for a name like deep-hyphenated-name which passes.

The travis build is currently failing because of the previous commit (not related to this).

@chancancode chancancode merged commit ee9e4c3 into rails:master Jan 30, 2015

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details
@chancancode
Member

@robinboening thanks again for this! 😄 FYI, I fixed a few more things in 33030ea

@robinboening
Contributor

Nice! @chancancode thanks for reviewing, merging and rounding it up. 👍

@robinboening robinboening deleted the robinboening:allow_dashes_in_gem_names branch Jan 31, 2015
@tvdeyen
Contributor
tvdeyen commented Jan 31, 2015

🎉👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment