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

Pages flag added for listing jobs based on page number #265

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

pranjalg1331
Copy link

@pranjalg1331 pranjalg1331 commented Jan 10, 2025

Description

I have added a new function get_jobs_by_page that returns a jobs list based on the page number.
Please include a summary of the change.

Related issues

Closes #264

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue).
  • [x ] New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist

  • [ x] The pull request is for the branch develop
  • I have added tests in the Tests folder.
  • The tests gave 0 errors.
  • [ x] Black gave 0 errors.
  • [ x] Flake gave 0 errors.
  • I squashed the commits into a single one. (optional, they will be squashed anyway by the maintainer)

please follow these rules

  • If your changes decrease the overall tests coverage (you will know after the Codecov CI job is done), you should add the required tests to fix the problem
  • Everytime you make changes to the PR and you think the work is done, you should explicitly ask for a review

Real World Example

Please delete if the PR is for bug fixing.
Otherwise, please provide the resulting raw JSON of a finished analysis (and, if you like, a screenshot of the results). This is to allow the maintainers to understand how the analyzer works.

Signed-off-by: pranjalg1331 <[email protected]>
@pranjalg1331
Copy link
Author

pranjalg1331 commented Jan 10, 2025

@fgibertoni Please review.
Also should I go ahead and add the function I have created in the docs here?

Copy link
Contributor

@fgibertoni fgibertoni left a comment

Choose a reason for hiding this comment

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

Hi! Thank you for your submission. I suggested only some small changes.
Also, I noticed that the unit tests covering your new code are missing, please add them.

Regarding your question, yes. I think that this new functionality should be added in the doc

@@ -581,6 +581,24 @@ def get_all_jobs(self) -> List[Dict[str, Any]]:
response = self.__make_request("GET", url=url)
return response.json()

def get_jobs_by_page(self, page: int) -> List[Dict[str, Any]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to align the parameter to the others.

Suggested change
def get_jobs_by_page(self, page: int) -> List[Dict[str, Any]]:
def get_jobs_by_page(self, page: Union[int, str]) -> List[Dict[str, Any]]:

@@ -42,12 +42,22 @@ def jobs():
show_choices=True,
help="Only show jobs having a particular status",
)
@click.option(
"-p",
"--page",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this parameter should be added so that the --help option of the ls command shows that it's expecting an interger and not text.

Suggested change
"--page",
"--page",
type=int,

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