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 Concerns section in Refactoring portion of Getting Started guide #39766

Merged
merged 5 commits into from Sep 22, 2020

Conversation

maryammouse
Copy link
Contributor

@maryammouse maryammouse commented Jul 1, 2020

Summary

  • Adds a new Concerns section to the Getting Started guide, underneath the Refactoring header
  • Updates a later code snippet to reflect the changes made in the Concern section

Why?

  • This addresses the need for documentation on concerns; a new Rails app contains concerns folders for controllers and models but their purpose is not explained.
  • This has been requested by many people over the years (see previous PR and corresponding Discourse thread)

Other Information

~ I am addressing feedback in new commits and will squash all before merging. :)

This commit:

* Adds a new Concerns section to the Getting Started guide, underneath the Refactoring header
* Updates a later code snippet to reflect the changes made in the Concern section

Why?

* This addresses the need for documentation on concerns; a new Rails app contains `concerns` folders for controllers and models but their purpose is not explained.
* This has been requested by many people over the years (see previous [PR](rails#21114) and corresponding [Discourse thread](https://discuss.rubyonrails.org/t/helping-devs-understand-concerns-faster/74619/44))
@rails-bot rails-bot bot added the docs label Jul 1, 2020
guides/source/getting_started.md Show resolved Hide resolved
guides/source/getting_started.md Outdated Show resolved Hide resolved
Copy link

@murdho murdho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'll be a great addition to the Getting Started guide! 👍 Also added a couple of tiny suggestions.

Cheers!

guides/source/getting_started.md Outdated Show resolved Hide resolved
guides/source/getting_started.md Outdated Show resolved Hide resolved
guides/source/getting_started.md Outdated Show resolved Hide resolved
guides/source/getting_started.md Outdated Show resolved Hide resolved
guides/source/getting_started.md Outdated Show resolved Hide resolved
guides/source/getting_started.md Outdated Show resolved Hide resolved
guides/source/getting_started.md Outdated Show resolved Hide resolved
@vipulnsward vipulnsward requested a review from fxn July 5, 2020 14:44
@maryammouse
Copy link
Contributor Author

maryammouse commented Jul 7, 2020

Thanks for all the feedback so far, everyone! I've made changes or asked for further guidance where I'm uncertain about the requested change.

(tagging @murdho, @santib, @fylooi since I don't seem to be able to use the re-request review feature - apologies for any duplicate tags! I have tagged dzunk in specific comments also.)

Copy link

@tvanderpol tvanderpol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I really like this addition to the docs, definitely a clear introduction to Concerns. Only had one tiny nit to pick.

guides/source/getting_started.md Outdated Show resolved Hide resolved
@maryammouse
Copy link
Contributor Author

@tvanderpol @fylooi your feedback has been addressed! :) Thanks for catching my mistake in the class method example 😊


### Using Concerns

A concern is any module that contains methods representing a well-defined slice of the functionality that a model or controller is responsible for. In other frameworks they are often known as mixins. Concerns are a way to make large controllers or models easier to understand and manage. This also has the advantage of reusability when multiple models (or controllers) share the same concerns.
Copy link

@fylooi fylooi Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While accurate, I'd hesitate to describe a concern as a module.

I think concerns in Rails are primarily a design approach / style / pattern to break down models into separate (and reusable) slices of responsibility. A concern is implemented using modules.

Here's a different approach which is also implemented with modules containing well defined slices of functionality

class Article
end

module Notifier
  def send_notifications
    ...
  end
end

class ArticlesController
  def create
    @article.extends(Notifier)

    if @article.save
       @article.send_notifications
    end
  end
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps reusing "a way to" makes it clearer that it's a pattern instead of tying it to modules?

Suggested change
A concern is any module that contains methods representing a well-defined slice of the functionality that a model or controller is responsible for. In other frameworks they are often known as mixins. Concerns are a way to make large controllers or models easier to understand and manage. This also has the advantage of reusability when multiple models (or controllers) share the same concerns.
A concern is a way to represent a well-defined slice of the functionality that a model or controller is responsible for. In other frameworks they are often known as mixins. They help make large controllers or models easier to understand and manage. This also has the advantage of reusability when multiple models (or controllers) share the same concerns.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps reusing "a way to" makes it clearer that it's a pattern instead of tying it to modules?

I like the flow, think the mixin line is more of a reference to modules though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fylooi @sunny I just made a new revision - Swapped around the order of the sentences. I'm not sure if the mixin line is still worth including or not as a definition of modules might not be needed in a Rails guide, but I've kept it so far because it seems like a useful comparison to make to quickly build up understanding for those that are new to Rails.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 👏

I'm not sure if the mixin line is still worth including

I'd be inclined to remove it. To me, mixins are how we call "includable modules" in Ruby. I'd say that concerns are mixins so I feel that "In other languages, modules are often known as mixins" doesn't quite fit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Ruby you split code in modules. If some module is reusable by several models, you put it in app/models/concerns. That's it.

On the other hand, Active Support has something called ActiveSupport::Concern that you do not need at all for writing concerns. A concern is a module. AS::Concern has some sugar for certain module inclusion patterns, and would have its own module documentation in the API.

And that is all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fxn When I first created this PR, given your previous feedback on the old PR, I actually had written the introduction as basically what you have here - concerns are modules, they help you reuse code. Does that seem like a better version than what is currently in the PR?

Aside from that, I think there's value in demonstrating how to use ActiveSupport::Concern or at least linking to the API docs from this guide. There's precedence for demonstrating the use of ActiveRecord or ActiveSupport in these guides - here and here -- even though they already have API guides - and from the feedback I'm seeing, there's value in having them as part of these guides as opposed to the API docs alone, which doesn't explicitly state the use for the folders.

For me personally, what it comes down to is that a basic Rails app contains empty folders within prominent directories (models and controllers) that are not explicitly explained. If the folders were called modules instead of concerns, that would be pretty self-explanatory (even though I would personally still wonder why they are included if they are empty, when each user can create modules in any directory structure they like). Their presence and the name concerns implies a Rails convention. So as long as the folders exist with a name that is Rails-specific, we should have documentation somewhere intuitive to explain them - whether that's a tutorial like what I've written here, or just a paragraph or two (which the previous PR was reduced to.) I totally agree with you that the Rails guides should not be responsible for demonstrating underlying Ruby concepts - but from what I can tell given the feedback I'm seeing, many people do not think concerns are just modules, and therefore they are not an underlying Ruby concept.

I've always considered the Rails guides to be where you go to get wider context around how, when, and why to use various Rails methods and conventions, whereas each page of the API guide is much more limited in scope. That is why I think the Rails guides should have some documentation for concerns (or remove the folders from Rails entirely, so we no longer imply a Rails convention if there isn't one). What the scope should be - I think that's the main challenge now. I can go either way - I think there is value in explaining the conceptual methodology, but if we can get a consensus on focusing these docs on modules without limiting the definition of concerns (as in @mcgregordan 's feedback) , I'm happy to go with that too. :)

Tagging @sunny and @fylooi also - sorry this became a bit of an essay!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've always considered the Rails guides to be where you go to get wider context around how, when, and why to use various Rails methods and conventions

This is also exactly what I think of the Rails guides at and the proposed documentation changes in this PR already function very well for this purpose - it's easy to see how the examples given would extend to other domains and your own problems you're using rails to solve.

I just don't want to see the added value of what's there get overlooked in the chase for something that's perhaps even better, especially if that version involves explaining less than the current version does.

Copy link

@fylooi fylooi Jul 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, Active Support has something called ActiveSupport::Concern that you do not need at all for writing concerns. A concern is a module. AS::Concern has some sugar for certain module inclusion patterns, and would have its own module documentation in the API.

And that is all.

While ActiveSupport::Concern can be used on its own, combining this sugar with the declarative DSL of an ActiveRecord model feels like a powerful and distinctively Rails convention to me. Think this particular pattern is worth highlighting as it is non-intuitive to beginners.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaspth @rafaelfranca what do you think of this PR so far? It would be great to hear if you share @fxn 's concerns around documentation or not - I made this PR based on feedback from others, but also your comments in my previous PR #21114 - so would be great to hear what you think! I haven't got any answer from a core team member since I responded.

Copy link
Contributor

@sunny sunny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome to see this coming to life in the guides! 🎉 Found some small nitpicks.

guides/source/getting_started.md Outdated Show resolved Hide resolved
guides/source/getting_started.md Outdated Show resolved Hide resolved
guides/source/getting_started.md Outdated Show resolved Hide resolved
guides/source/getting_started.md Outdated Show resolved Hide resolved
@maryammouse
Copy link
Contributor Author

@sunny Good eye, thanks for spotting those nits! I've gone ahead and updated the guide so those should all be gone now. :)

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another round of comments. (@maryammouse — you're going through quite the ordeal ❤️).

These are mostly around boxing off what we want this guide to do. I understand the rationale for only talking about concerns as something you reuse between models or controllers. But if we do decide to cover just that and not the richer case for concerns (for separation — almost intellectual separation — of discrete bits of code), I think we could do with being a bit more precise. We certainly don't want to suggest concerns are only for that and I'm pretty keen we don't give that impression in any way at all.


### Using Concerns

Concerns are a way to make large controllers or models easier to understand and manage. This also has the advantage of reusability when multiple models (or controllers) share the same concerns. Concerns are implemented using modules that contain methods representing a well-defined slice of the functionality that a model or controller is responsible for. In other languages, modules are often known as mixins.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a stab at tightening up the wording to box off what this guide is going to discuss (if we do decide to only cover concerns as reusable modules) while being careful not to put any limits on what a concern can be.

Suggested change
Concerns are a way to make large controllers or models easier to understand and manage. This also has the advantage of reusability when multiple models (or controllers) share the same concerns. Concerns are implemented using modules that contain methods representing a well-defined slice of the functionality that a model or controller is responsible for. In other languages, modules are often known as mixins.
Concerns are a way to make large controllers or models easier to understand and manage. They are implemented using modules that represent a slice of functionality. (In other languages, modules are often known as mixins.) Here, we are going to explore a particularly powerful way to use concerns: using them to share behaviour between different models or different controllers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the current wording.


Concerns are a way to make large controllers or models easier to understand and manage. This also has the advantage of reusability when multiple models (or controllers) share the same concerns. Concerns are implemented using modules that contain methods representing a well-defined slice of the functionality that a model or controller is responsible for. In other languages, modules are often known as mixins.

You can use concerns in your controller or model the same way you would use any module. When you first created your app with `rails new blog`, two folders were created within `app/` along with the rest:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd only put reusable concerns in concerns/. We might only discuss reusable concerns here, so that's all good — but I think we should still be careful to imply concerns/ isn't the only place for concerns. I extract concerns like a butcher on a farm and litter concerns throughout app/, app/models especially.

Suggested change
You can use concerns in your controller or model the same way you would use any module. When you first created your app with `rails new blog`, two folders were created within `app/` along with the rest:
You use concerns the same way you use any module, but Rails gives you a neat place to put them when you have a concern you want to use in different models or different controllers. When you first created your app with `rails new blog`, two folders were created in `app/` for reusable concerns:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also prefer the current wording.

guides/source/getting_started.md Show resolved Hide resolved
guides/source/getting_started.md Show resolved Hide resolved
Copy link
Contributor

@santib santib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope this time the PR gets merged, good luck 🍀

@maryammouse
Copy link
Contributor Author

Thanks @santib , I hope so too! ❤️

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the content of this section of the guide. While I agree with @fxn that concerns are just ruby modules and it is not the responsability of the guides to teach Ruby features, I think this section in specific is focusing on how to and why use ActiveSupport::Concern while also presenting the concern folders.


### Using Concerns

Concerns are a way to make large controllers or models easier to understand and manage. This also has the advantage of reusability when multiple models (or controllers) share the same concerns. Concerns are implemented using modules that contain methods representing a well-defined slice of the functionality that a model or controller is responsible for. In other languages, modules are often known as mixins.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the current wording.


Concerns are a way to make large controllers or models easier to understand and manage. This also has the advantage of reusability when multiple models (or controllers) share the same concerns. Concerns are implemented using modules that contain methods representing a well-defined slice of the functionality that a model or controller is responsible for. In other languages, modules are often known as mixins.

You can use concerns in your controller or model the same way you would use any module. When you first created your app with `rails new blog`, two folders were created within `app/` along with the rest:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also prefer the current wording.

@fxn fxn merged commit 364c215 into rails:master Sep 22, 2020
@ghiculescu
Copy link
Member

ghiculescu commented Sep 23, 2020

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

10 participants