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

Update documentation for QueueAdapters::lookup #20371

Merged
merged 1 commit into from
Jun 1, 2015

Conversation

davydovanton
Copy link
Contributor

I added simple documentation for method and also added :nodoc: because this method called only in private QueueAdapter::interpret_adapter.

/cc @kaspth

@kaspth
Copy link
Contributor

kaspth commented May 29, 2015

I'm not sure about nodoc'ing it. While it's true it's only used one place internally, other people might use it in their apps because it's been public API.

Also with the nodoc people will only see the comment right next to the implementation and I feel like that's communicative enough in and of itself.
But ¯_(ツ)_/¯, what do you think @chancancode @rafaelfranca?

@senny
Copy link
Member

senny commented May 31, 2015

Agree with @kaspth either we :nodoc: it but then we don't need the comment (code is pretty clear) or we add the comment for the public API.

@davydovanton
Copy link
Contributor Author

@kaspth, @senny I agree with you, and I deleted nodoc

@kaspth
Copy link
Contributor

kaspth commented May 31, 2015

Let's add an example too, since now people don't have the implementation next to them.

@davydovanton
Copy link
Contributor Author

@kaspth added simple example for this method. What do you think?

# Returns adapter for specified name. For example, this enables to perform
# job with specific adapter:
#
# Adapter = ActiveJob::QueueAdapters.lookup(:sidekiq)
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 the variable assignment is necessary

@davydovanton
Copy link
Contributor Author

@kaspth @senny updated all 😅

senny added a commit that referenced this pull request Jun 1, 2015
Update documentation for QueueAdapters::lookup [ci skip]
@senny senny merged commit 2db8414 into rails:master Jun 1, 2015
@senny
Copy link
Member

senny commented Jun 1, 2015

Thank you 💛

@davydovanton
Copy link
Contributor Author

@senny @kaspth thank you for review my PR 🎉 🌟 🎉

@davydovanton davydovanton deleted the doc-adapter-lookup branch June 1, 2015 11:34
@kaspth
Copy link
Contributor

kaspth commented Jun 1, 2015

@davydovanton Nice ❤️

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

Successfully merging this pull request may close these issues.

None yet

3 participants