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

Add Feature in format-checker: RepoArchived-check #383

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

LeoYimingLi
Copy link
Contributor

(a) Changes overview:

  • format_checker/common_checks.py
    • check the archived repo in proposed PR without RepoArchived status
  • format_checker/pr_checker.py
    • check the non-archived repo in proposed PR but with RepoArchived status
  • format_checker/requirements.txt
    • add one dependency
  • format_checker/utils.py
    • add a log function for repo-archived error

(b) Some test results showing the changes work by running python3 format_checker/main.py

  1. check the archived repo in proposed PR without RepoArchived status
    image

  2. check the non-archived repo in proposed PR but with RepoArchived status
    image

it should report error when a repo with status 'repoArchived' but it 's not archived.
add one dependency: requests
add log function for archived status error
@LeoYimingLi LeoYimingLi changed the title Add RepoArchived check for PR Add RepoArchived check in format-checker Dec 4, 2021
@LeoYimingLi LeoYimingLi changed the title Add RepoArchived check in format-checker Add RepoArchived-check in format-checker Dec 4, 2021
@LeoYimingLi LeoYimingLi changed the title Add RepoArchived-check in format-checker Add Feature in format-checker: RepoArchived-check Dec 5, 2021
Copy link
Contributor

@winglam winglam left a comment

Choose a reason for hiding this comment

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

As this PR is quite similar to #334 I would prefer merging that one first and then for some caching of requests.get(project_url) to be used in this PR so that at most we only make one request for each unique Project URL instead of making one for fork checking, one for repo archive, etc.

Thanks for your changes in the meantime!

log_archived_error(filename, log, i, row, "Project URL")
except requests.exceptions.RequestException as e:
# handle(e)
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of simply passing here, it would be good to warn that the check for repo archived failed along with outputting the exception

Copy link
Contributor Author

@LeoYimingLi LeoYimingLi Dec 7, 2021

Choose a reason for hiding this comment

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

get it, I change pass to log_esp_error(filename, log, "The check for repo-archived failed due to ERROR:" + str(e))

yes the PR is similar to #334, maybe the two funcitions can merge into one ?

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