Skip to content

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