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

Several ActionCable doc fixes #23429

Merged
merged 1 commit into from Feb 2, 2016
Merged

Conversation

qrush
Copy link
Contributor

@qrush qrush commented Feb 2, 2016

Correcting indentation, grammar, and a few other nits. 🔏

@rails-bot
Copy link

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

# "web_notifications_1", { title: 'New things!', body: 'All shit fit for print' }
# # Somewhere in your app this is called, perhaps from a NewCommentJob
# ActionCable.server.broadcast \
# "web_notifications_1", { title: 'New things!', body: 'All that's fit for print' }
Copy link
Member

Choose a reason for hiding this comment

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

🎱

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, didn't spot this before: unescaped quote

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, fixed + force pushed (again).

On Tue, Feb 2, 2016 at 2:26 PM, Matthew Draper notifications@github.com
wrote:

In actioncable/lib/action_cable/server/broadcasting.rb
#23429 (comment):

 #
  • # Somewhere in your app this is called, perhaps from a NewCommentJob

  • ActionCable.server.broadcast \

  • "web_notifications_1", { title: 'New things!', body: 'All shit fit for print' }

  • # Somewhere in your app this is called, perhaps from a NewCommentJob

  • ActionCable.server.broadcast \

  • "web_notifications_1", { title: 'New things!', body: 'All that's fit for print' }

Sorry, didn't spot this before: unescaped quote


Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/23429/files#r51620418.

@vipulnsward
Copy link
Member

@qrush this needs a squash of commits + [ci skip] in the commit message. Thanks!

@@ -32,7 +32,7 @@ module Channel
#
# == Action processing
#
# Unlike Action Controllers, channels do not follow a REST constraint form for its actions. It's a remote-procedure call model. You can
# Unlike ActionControllers, channels do not follow a REST constraint form for their actions. It's a remote-procedure call model. You can
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we've ever used ActionControllers (with or without the space) to mean "instances of AC::Base". It feels awkward to me, but I don't have a concrete suggestion for an alternative. 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, felt weird to me too. Reworked to say "subclasses of".

@qrush qrush force-pushed the actioncable-doc-update branch 2 times, most recently from 80ebf45 to c8e782f Compare February 2, 2016 17:01
@qrush
Copy link
Contributor Author

qrush commented Feb 2, 2016

Rebased. 🔪

# Unlike Action Controllers, channels do not follow a REST constraint form for its actions. It's a remote-procedure call model. You can
# declare any public method on the channel (optionally taking a data argument), and this method is automatically exposed as callable to the client.
# Unlike subclasses of ActionController::Base, channels do not follow a REST
# constraint form for their actions. It's a remote-procedure call model. You
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead, ActionCable operates through a remote-procedure call model.

@maclover7
Copy link
Contributor

Few small grammar notes 😄

@qrush
Copy link
Contributor Author

qrush commented Feb 2, 2016

Rebased again + a few more nits from @maclover7. I think this is ready to merge.

* Properly indent code sample in ActionCable::Channel::Streams
* Add a doc comment for #stop_all_streams
* Reformat + add <tt> blocks around code references in ActionCable::Base docs
* Clarify and a little better grammar on ActionCable::RemoteConnections
* Correct indentation and clean up ActionCable::Server::Broadcasting code sample
eileencodes added a commit that referenced this pull request Feb 2, 2016
@eileencodes eileencodes merged commit d34db3d into rails:master Feb 2, 2016
@rafaelfranca
Copy link
Member

Thanks!!!!

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

Successfully merging this pull request may close these issues.

None yet

8 participants