Skip to content

Conversation

@Ankit-lab809
Copy link
Collaborator

No description provided.

Copy link

@sachin-mehta sachin-mehta left a comment

Choose a reason for hiding this comment

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

@Ankit-lab809 I have added my comments.

Choose a reason for hiding this comment

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

Suggestion: add a simple validation check (either at startup or task execution) to fail fast with a clear error message if required config values are missing. @Ankit-lab809

Choose a reason for hiding this comment

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

@Ankit-lab809, consider breaking this big function into smaller helper functions to improve readability and unit testability.

Choose a reason for hiding this comment

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

@Ankit-lab809 I think we should make these hardcoded values, like soft_limit, limit, batch_size etc. as constants/configurable because same hardcoded values must be present in School logic as well. So, in future, if something needs to be changed, we can change in one place only.

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