Skip to content

Comments

Add perform_all_later to Active Job Guide#2

Open
bhumi1102 wants to merge 39 commits intomainfrom
bhumi-guides-active-job
Open

Add perform_all_later to Active Job Guide#2
bhumi1102 wants to merge 39 commits intomainfrom
bhumi-guides-active-job

Conversation

@bhumi1102
Copy link
Owner

@bhumi1102 bhumi1102 commented Feb 2, 2024

Overview

Add documentation related to perform_all_later for bulk enqueuing jobs.

Testing

Tested the new method in-app using sidekiq locally. Also tested documentation by running guides:generate, guides:validate and guides:lint locally.

Copy link
Collaborator

@Ridhwana Ridhwana left a comment

Choose a reason for hiding this comment

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

This looks great! ✨ I left a few suggestions and comments and marked them as optional etc.

Copy link
Collaborator

@carlosantoniodasilva carlosantoniodasilva left a comment

Choose a reason for hiding this comment

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

👍 looking good, just some minor bits of initial feedback.

bhumi1102 and others added 10 commits February 6, 2024 10:38
Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
@bhumi1102
Copy link
Owner Author

I've addressed all review feedback. Anything else before moving to PR on rail/rails? Oh, I'm up for moving the location of section 3.3, let me know if you have a strong preference to where?

Copy link
Collaborator

@carlosantoniodasilva carlosantoniodasilva left a comment

Choose a reason for hiding this comment

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

Looking great! I did another quick read through, and had another few minor suggestions, one typo in the multiple classes example.

I'm up for moving the location of section 3.3, let me know if you have a strong preference to where?

Not necessarily a strong preference, but there's a couple of reasons we might want to consider moving it to at least after the callbacks section:

  • It has its own little section on callbacks, which links down the guide, and then there's a sub-section on callbacks with bulk enqueuing as well. Moving it down probably means we can merge the two into a single one.
  • It explains the need to be backed by a backend queue, but we haven't gotten to the point of explaining what backend queues are yet. Moving it down would put it after that concept explanation.

I can also see why keep it early in the guide, within the section we explain about how to create and enqueue jobs. But it could be treated more as an "advanced" concept / form of enqueueing as well.

That doesn't need to stop you from moving the PR to rails/rails, maybe could even ask the broader community for feedback on that specifically if still unsure. We might still review the Active Job guide as a whole later on as well.

bhumi1102 and others added 5 commits February 7, 2024 10:43
Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
bhumi1102 and others added 3 commits February 7, 2024 10:48
Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
@bhumi1102
Copy link
Owner Author

I can also see why keep it early in the guide, within the section we explain about how to create and enqueue jobs. But it could be treated more as an "advanced" concept / form of enqueueing as well.

That doesn't need to stop you from moving the PR to rails/rails, maybe could even ask the broader community for feedback on that specifically if still unsure. We might still review the Active Job guide as a whole later on as well.

I moved the whole section 3.3 to a new top level section 8. And linked the new section within 3.3. Also cleaned up the 'callbacks' docs in multple places. Seems good for now. Going over the entire Active Job guide later on sounds good!

Will create a rails/rails PR now!

bhumi1102 and others added 12 commits February 14, 2024 20:38
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Juanjo Bazán <jjbazan@gmail.com>
Co-authored-by: Juanjo Bazán <jjbazan@gmail.com>
Co-authored-by: Juanjo Bazán <jjbazan@gmail.com>
Co-authored-by: T.J. Schuck <tj@tjschuck.com>
Co-authored-by: T.J. Schuck <tj@tjschuck.com>
@bhumi1102 bhumi1102 mentioned this pull request Apr 26, 2024
11 tasks
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.

3 participants