Move changeset comments feed to resourceful routes

[AntonKhorev]

Changeset comments as nested resources of changesets. That gives path helper names like feed_changeset_changeset_comments_path. At first I tried renaming "changeset_comments" to just "comments", but that didn't work because there's also a feed of all changeset comments and it takes feed_changeset_comments_path.

[gravitystorm]

This feels like a "one step forward, one step back" PR to me unfortunately.

There's two parts in moving to resourceful routing, one part is using resources declaration in the routing file, but the other part is trying to have resourceful controller methods i.e. just index/show/new/create/edit/update/destroy , which the resources declarations use by default.

In #2050 (specifically 4b0d56f) I renamed the comments_feed (which is what was used when the code was in changesets_controller) to the index method, since that's what most closely aligns with what this doing i.e. showing a list of objects. So I'd like to keep this as a method named index.

I realise that we still have feed methods elsewhere, but those are targets for refactoring too. There's a difficulty when there are already index methods that do different things (e.g. different lists of objects, or search filters, or other things, so in those cases it can't be as simple as just a respond_to switch) but in those cases I would prefer to see e.g. FooController::index and Foo::FeedController::index or similar, i.e. the method name is still index.

See http://jeromedalbert.com/how-dhh-organizes-his-rails-controllers/ for more info.

[AntonKhorev]

In #2050 (specifically 4b0d56f) I renamed the comments_feed (which is what was used when the code was in changesets_controller) to the index method, since that's what most closely aligns with what this doing i.e. showing a list of objects. So I'd like to keep this as a method named index.

And I do the opposite thing in #4248 because I need the actual index path mapped to the actual index method.

I realise that we still have feed methods elsewhere, but those are targets for refactoring too. There's a difficulty when there are already index methods that do different things (e.g. different lists of objects, or search filters, or other things, so in those cases it can't be as simple as just a respond_to switch) but in those cases I would prefer to see e.g. FooController::index and Foo::FeedController::index or similar, i.e. the method name is still index.

Then that has to be done in all cases where feeds have their own paths.

[gravitystorm]

Then that has to be done in all cases where feeds have their own paths.

Yes, I think so. Unless there are alternative ideas - I'm willing to discuss options.

[AntonKhorev]

Renamed ChangesetCommentsController to ChangesetCommentsFeedsController and mapped it to a singular comments/feed resource.

[gravitystorm]

Sorry for the delay in re-reviewing!

[gravitystorm]

As before, I don't think this is the correct action. This isn't a single object, it's a list of objects, so the method should be index to reflect this.

[AntonKhorev]

You could have easily argued for the opposite thing, which I'm not going to do because I just want to move feeds out of the way of #4248.

[gravitystorm]

I've spent some time considering your point about whether we are :showing a feed, or :indexing a list of changeset comments. In doing so, I've realised that I think the module naming in this PR might be the wrong way round, i.e. Feeds::ChangesetCommentsController vs ChangesetComments::FeedsController.

But with everything related to routes.rb, the only way I can be sure is by messing around and seeing what happens, and now I have an alternative approach ready that I prefer, but I'm not sure what you will think of it.

So what I'm going to do is merge this PR, so that it unblocks your work, and then make my own followup PR for you to review.