Skip to content

Implement list_jobs method in scheduler#83

Open
Shouryaverma19 wants to merge 11 commits into
Code-Society-Lab:mainfrom
Shouryaverma19:patch-1
Open

Implement list_jobs method in scheduler#83
Shouryaverma19 wants to merge 11 commits into
Code-Society-Lab:mainfrom
Shouryaverma19:patch-1

Conversation

@Shouryaverma19

Copy link
Copy Markdown

Added a method to list names of all registered jobs.

Added a method to list names of all registered jobs.

@PenguinBoi12 PenguinBoi12 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You need to add tests for this. Usually there should be a test_scheduler but it seems like it was skip when we worked on that so it's a good opportunity to add it. It will be useful for the #71 Otherwise, it looks good to me.

Add tests for Scheduler's list_jobs method to verify it returns an empty list and proper format.
@Shouryaverma19

Copy link
Copy Markdown
Author

Hey @PenguinBoi12! I have switched branches to patch-1 and successfully created the missing test_scheduler.py file inside the tests directory to verify the list_jobs logic. Could you please approve and trigger the workflow tests to verify the suite when you get a chance?

Comment thread tests/test_scheduler.py
Comment thread tests/test_scheduler.py Outdated
Comment thread tests/test_scheduler.py Outdated
Comment thread tests/test_scheduler.py Outdated
@PenguinBoi12

Copy link
Copy Markdown
Contributor

Hey @PenguinBoi12! I have switched branches to patch-1 and successfully created the missing test_scheduler.py file inside the tests directory to verify the list_jobs logic. Could you please approve and trigger the workflow tests to verify the suite when you get a chance?

You should run the tests, mypy and black locally before pushing to make sure the workflow will pass:

  • Run tests: pytest tests/
  • Run type check: mypy matrix/
  • Run formatting: black .

@PenguinBoi12 PenguinBoi12 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to have a test that validates that pour list works correctly.

You didn't follow the projects formatting, you can run black . to format the changes you've made. Without that the CI won't pass.

Comment thread tests/test_scheduler.py

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Removing the test is not an appropriate way to fix the issue. Now the feature isn't fully tested.

@PenguinBoi12 PenguinBoi12 added the feature A new feature label Jul 2, 2026
@PenguinBoi12

Copy link
Copy Markdown
Contributor

Hey @Shouryaverma19, I see that you tried to fix the formatting. The easiest what is to run black . locally. It's going to format everything automatically.

Additionally, you did not address the issue with the tests. We should have a test that actually validate the result when jobs are listed.

Thank you and if you need anything let me know.

Add method to unschedule jobs by name.
Added a sample task function and expanded the test to include job management scenarios.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scheduler: Add list_jobs method

3 participants