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

Mention Active Job integration in the README #231

Merged
merged 4 commits into from
Oct 29, 2019
Merged

Mention Active Job integration in the README #231

merged 4 commits into from
Oct 29, 2019

Conversation

giovannibonetti
Copy link
Contributor

@giovannibonetti giovannibonetti commented Oct 15, 2018

Thank you for this great library!

I've read the discussion on #127 and it seems like now there is great Active Job integration, so maybe it would be nice to document it so that new users can easily find it and decide to use it.

It took me a while to find this library because it is not mentioned in the Rails Guides, even though it is mentioned in the API docs. It seems like the guides are more beginner-friendly and have links specific to the README section explaining how to integrate the library with Active Job. Hence if we have that section in the README we could have a link from Rails Guides pointing directly to it which would bring people here.

What do you think?

@siegy22
Copy link
Member

siegy22 commented Sep 23, 2019

Hey there!

Que moved: https://github.com/que-rb/que
Can you reopen your PR on the repository there so we can have a look at it?

(Automatically transfered)

@giovannibonetti
Copy link
Contributor Author

giovannibonetti commented Sep 24, 2019

Hi! It seems we are already on the new repository: #231

Doesn't it work the way it is now?

@siegy22
Copy link
Member

siegy22 commented Sep 24, 2019

Yeah forget my comment, we first forked the repository but then decided to transfer it to the organization.

Copy link
Member

@siegy22 siegy22 left a comment

Choose a reason for hiding this comment

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

My opinion (and goal) on this for que 1.0:

  • We should provide a rails generator to install que, something like rails g que:install which should generate the migration and change the active record dump to structure.sql.
  • We should have clear documentation paths what's rails specific and what's not.

(Just a general notice for myself here) 😄

README.md Outdated Show resolved Hide resolved
README.md Outdated
ActiveJob::Base.queue_adapter = :que
```

Que will automatically pick up the database from Active Record, so there is no need to configure anything else.
Copy link
Member

Choose a reason for hiding this comment

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

I think you mean Active Job, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I read this part of the code I thought the database connection was managed by Active Record. In fact I think you can use Que with Active Record without using Active Job at all, but I might be wrong.

Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Ah. Now I get it. This says that que will automatically use the Active Record connection.

I think the pick up is confusing. Can we use something like:

Que will automatically use the database configuration of your rails application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Anything else?

README.md Outdated Show resolved Hide resolved
@siegy22
Copy link
Member

siegy22 commented Oct 29, 2019

@giovannibonetti Maybe you missed my feedback on the second part. So see this message as a friendly "ping", because I'm really looking forward to have this in the README!

@giovannibonetti
Copy link
Contributor Author

All right, I didn't notice that part. I will change it now. 👍

@siegy22
Copy link
Member

siegy22 commented Oct 29, 2019

I'm not a native english speaker, so if you disagree, feel free to keep it as is.

@giovannibonetti
Copy link
Contributor Author

I'm not either, but your suggestion looks good, so I think it is worth the change! 👍

@siegy22
Copy link
Member

siegy22 commented Oct 29, 2019

Thanks for your contribution! 💚

@giovannibonetti
Copy link
Contributor Author

You're welcome! 😀

@siegy22 siegy22 merged commit c3547f5 into que-rb:master Oct 29, 2019
@giovannibonetti giovannibonetti deleted the patch-1 branch October 29, 2019 15:20
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.

2 participants