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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add module/class nesting consistency on newly generated application: #44393

Merged
merged 1 commit into from
Feb 10, 2022

Conversation

Edouard-chin
Copy link
Member

Hello 馃憢 , long time 馃槂

Hoping to get this small change accepted even though it is style related.

@rails-bot rails-bot bot added the railties label Feb 10, 2022
- These two generated files are the only two that uses a compact style
  when defining a class, (it's not the case for their non-test
  counterpart i.e. https://github.com/rails/rails/blob/0f3642596f41bb323772fb7d160a3acdf0498650/railties/lib/rails/generators/rails/app/templates/app/channels/application_cable/connection.rb.tt#L1-L2).

  This fixes that to add some consistency and will avoid users using
  rubocop with the `Style/ClassAndModuleChildren: nested` rule to have
  to manually edit those files when generating a new app.
@gmcgibbon
Copy link
Member

As much as I see the value in this change, Rails is compact is by default, so I don't think we can accept this change. This is contrary to many people's choices, but a style preference I don't think we can selectively choose not to follow. Sorry about that! 馃槮

@gmcgibbon gmcgibbon closed this Feb 10, 2022
@Edouard-chin
Copy link
Member Author

Edouard-chin commented Feb 10, 2022

I don't think that Rails by default is compact, every generator's templates uses the module_namespacing thing to nest classes (i.e.

<% module_namespacing do -%>
class <%= class_name %>Job < ApplicationJob
).

This is contrary to many people's choices,

No matter what people choices are, when you generate a new app you'd have to fix the styling of one or the other since the style in the generated files is inconsistent.

@gmcgibbon gmcgibbon reopened this Feb 10, 2022
@gmcgibbon gmcgibbon merged commit 965367e into rails:main Feb 10, 2022
@gmcgibbon
Copy link
Member

gmcgibbon commented Feb 10, 2022

Use of module_namespacing is a good point. Also the fact that ActionCable::Connection is nested makes sense. I thought we leaned more toward compact style, but it seems a bit controversial. Thanks for contributing 鉂わ笍

@Edouard-chin
Copy link
Member Author

Thanks Gannon! Hope all is well :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants