-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Fixes requests specs generated by scaffold with namespace resource #2492
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
Conversation
I think this makes sense, @pirj / @benoittgt can either of you cast an eye over this? |
@@ -104,7 +104,7 @@ | |||
before { run_generator %w[admin/posts] } | |||
it { is_expected.to exist } | |||
it { is_expected.to contain(/^RSpec.describe "\/admin\/posts", #{type_metatag(:request)}/) } | |||
it { is_expected.to contain('admin_post_url(admin_post)') } | |||
it { is_expected.to contain('admin_post_url(post)') } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why?
accounts_user = Accounts::User.create! valid_attributes
get accounts_user_url(accounts_user), as: :json
It makes it clear what namespace a certain model belongs to.
Accounting::User
and Reader::User
might be quite different.
If RSpec is used as a project documentation tool, I'd prefer it to be more specific by default.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before my changes the generator output was:
# Output
user = Accounts::User.create! valid_attributes
get accounts_user_url(accounts_user), as: :json
# Source
# <%= file_name #> returns 'user'
<%= file_name %> = <%= class_name %>.create! valid_attributes
get <%= show_helper %>, as: :json
To keep the pattern from another templates, example controller_spec ,i have use the file_name to be argument from path. In my pratice i use more the small name like user, but thinking as project documentation tool is better keep the full_name from resource, do you like that i change all file_name
calls from generator source to be ns_file_name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JonRowe @benoittgt What do you guys think? accounts_user
or just user
for Accounts::User
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They should be the same and the spec should work either way? e.g. I think this report is that one is user
and one is accounts_user
and thats wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I explained the problem with more details in this issue #2491 . We can fix the spec generator to use accounts_user
or just user
, in my PR, I chose user
.
In order to change to accounts_user
and keep the pattern from another templates generators, we need to change some other lines.
@pirj did you guys make a choice? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on a newly-created app, works fine.
Thanks for your contribution, @PedroAugustoRamalhoDuarte, and please accept my apologies for the confusion regarding the requirements and the delayed response.
Description
Refactor show_helper to match variable spec data in requests specs created by scaffold generators, when the resource has a namespace
Example generator response
rails g scaffold accounts/user name:string
Issues
closes #2491