Add ability to namespace a resource for nifty:scaffold #63

Merged
9 commits merged into from Jan 17, 2011

6 participants

@DouglasMeyer

Hey Ryan-

This is in response to issue #7
Hopefully the "feature" test will show what this is trying to accomplish. So far, it only works in rails 3; I just found-out how to run the tests for rails 2. Could you let me know if this patch would be something you're interested in, or what you would like to see changed to make it acceptable?

Thanks,
-Doug

@ryanb
Owner

Thank you for your work on this Doug, sorry for not getting back to you sooner on this. It is something that I have been meaning to add to Nifty Generators but never got to it.

One thing I am unsure about is whether the model should have a namespace. I realize this is how the built in Rails scaffold generator works but imagine you are creating an administration interface for models. The models should often be at the base layer without a namespace.

What do you think? I'm adding a discuss label to this to get some feedback from others as well.

Your solution looks very clean and simpler than I had expected, nice job.

@DouglasMeyer

Ryan-
Thanks for going over this change; it's nice to hear that this is something you were meaning to get to.

I did think about the model namespacing, especially when the issue with the database table name came around; I figured in most cases, you would want to skip the model (with --skip-model). Say you already built a scaffold for Contacts, if you wanted to create an admin interface, you wouldn't create a new Admin::Contact model.

I imagine this would be more common than a case where you wouldn't have an existing model and would not want it namespaced. Also; with this approach, you can skip the model and build it with another generator; with the other approach, if you wanted one, you wouldn't be able to generate a namespaced model.

Thanks again for going over this. Let me know what you think,
-Doug

@ryanb
Owner

The only problem with the --skip-model option is that it will still reference the model name with the namespace in the controller. I prefer to have it only namespace the controller by default.

Maybe add a --namespace-model boolean option to get the behavior you have now? If it's too much trouble you can leave off the option and just remove the namespace on the model since I think that is generally the behavior one wants.

@ryanb
Owner

Actually I take that back, it isn't necessary to have the model namespace in the controller since Ruby will find it in the same namespace by default.

I would still like no model namespace as the default behavior though, so an option might still be necessary?

@DouglasMeyer

In the latest intermediate commit (DouglasMeyer@eb12c14), I'm seeing a weird issue. It looks like, in the Admin::UserController, the User model isn't getting loaded. Running the features should show this. So it looks like we may have to explicitly reference Admin::User to get to the model, when using the --namespace-model option. Otherwise, it should be functional.
PS: this is an intermediate commit, I'll finish it probably tomorrow and this commit will get blown-away. I think I've made it a personal vendetta to get this finished before the new year :-)

@ryanb
Owner

Thanks Douglas. Once you get this working I'll pull it in.

@DouglasMeyer

Hey Ryan-

it's working!! DouglasMeyer@7dabe27
After creating a new project, adding nifty generator to the Gemfile, and committing it in git; this command is a good test:
git checkout . && git clean -d -f && rails g nifty:scaffold Admin::User name:string && rails g nifty:layout -f && rails g nifty:scaffold Admin::User -f && bundle install && rake db:drop db:create db:migrate test
And there is the --namespace-model counterpart:
git checkout . && git clean -d -f && rails g nifty:scaffold Admin::User name:string --namespace-model && rails g nifty:layout -f && rails g nifty:scaffold Admin::User -f --namespace-model && bundle install && rake db:drop db:create db:migrate test

Thanks for following the progress on this.
-Doug

@ryanb
Owner

Thanks for your work on this. I will try to get it pulled in soon.

@noctivityinc

Ryan - Please let us know when you incorporate this. Ive been waiting for this for a LONG time :)

@ryanb
Owner

This is pulled in now and will be in a version release soon. Thanks Doug!

@DouglasMeyer

Your welcome. Glad I was able to help.

@tjobrien

Great stuff. I tried the following with Rails 2.3.11 and get the following result.
script/generate nifty_scaffold Admin::Account name:string
Result:
exists app/models
create app/models/admin/account.rb
No such file or directory - /Users/tom/rails/di_portal/app/models/admin/account.rb
Perhaps it only works with Rails 3?

Thanks,

Tom

@DouglasMeyer

Hey Tom & Ryan-

Sorry, oversight my me. I assume nifty-generators still supports rails 2 to some extent, so I'll see if I can get that working as well.

Let me know if that is not the case.
-Doug

@DouglasMeyer DouglasMeyer reopened this Apr 13, 2011
@ryanb
Owner

I am not planning on adding new features to the Rails 2 version of Nifty Generators. Partly because the tests are a mess. Doug, if you want to figure it out and make sure it's fully tested I will pull it in. But if you aren't up for it then don't worry.

@DouglasMeyer

Hey Tom-
I got a fix so nifty_scaffold with a nested resource works in Rails 2.
Ryan-
Doing this because "Rails 2" is mentioned at the beginning of the Readme with no mention of it being deprecated.

Let me know if it looks like anything is missing,
-Doug

@ryanb
Owner

@DouglasMeyer Thanks for your work on this, I'll get it pulled in soon.

@mfolnovic

I tried this, and seems to work in app. But, while running generated specs, I get:

/media/code/test/spec/controllers/articles/posts_controller_spec.rb:1:in `require': no such file to load -- /media/code/test/spec/controllers/articles/../spec_helper (LoadError)

I generated it with:
rails generate nifty:scaffold "Articles/Post" title:string content:text --haml --rspec

@mfolnovic

Also, running cucumber tests gets me https://gist.github.com/1033291

@mfolnovic

and it would be great if created table was articles_posts, not posts (in my example above) :)

@DouglasMeyer

Hey Matija-
Thanks for catching that issue with generating rspec tests. I've updated my fork, and we'll see if Ryan has any more time to update github (it looks like he has been busy).
I'm getting a few errors from running the tests, we may be using different versions of libraries. I would make sure you have the correct versions of the gems "bundle install" and are intentionally running them ("bundle exec rake" or "bundle exec cucumber").
As for saving to the "articles_posts" table, there is a flag "--namespace-model" that will do that and store the model Articles::Post in "app/models/articles/post.rb" (similar issues were discussed earlier in this pull request).

Hey Ryan-
Looks like this pull-request is getting kind of lengthy. My fork should only contain these bug fixes and related refactorings. But there is a commit to include a development dependency.
It looks like a few people (myself included) are having issues with failing/broken tests. Could you make sure things are running correctly (bundle install && bundle exec rake)? If not, I could look into it.
Thanks,
-Doug

@ryanb
Owner

@DouglasMeyer are you running the specs for the Rails 2 generator? Those are terribly broken. I am surprised you can get them to run at all actually, which is one reason I have not changed the Rails 2 generators much. I think I will discontinue Rails 2 support after this latest feature is in. Thanks for your work on it.

@DouglasMeyer

@ryanb Oddly enough, I didn't have many issues running rvm use ruby-1.8.7-p334@rails2 ; ruby -I . test/test_nifty_scaffold_generator.rb. I was talking about the tests that get run through bundle exec rake.
I think discontinuing support for rails 2 would immensely help with keeping the code clean. Probably say something like "Rails 2 supported up to version 0.4.6" in the Readme.
Thanks

@ryanb
Owner

Probably say something like "Rails 2 supported up to version 0.4.6" in the Readme.

Yeah, that is the plan. It is probably best to test Rails 2 additions manually for now, I don't trust the test suite that's in there for that. Once this release is done I will remove the Rails 2 support and only update it for minor bug fixes.

@jaylevitt

Bump! Would love to see this in nifty...

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