Refactor: Improve get_vale_conf script for structure and error handling#358
Conversation
akcano
left a comment
akcano
left a comment
There was a problem hiding this comment.
Looks good, but I think some changes may be needed.
| if download_file(item["download_url"], output_file): | ||
| success_count += 1 | ||
|
|
||
| print(f"Downloaded {success_count} items") |
There was a problem hiding this comment.
This may prove cumbersome with the recursion above; we don't get a total count but print separate numbers per each dir instead
| def download_file(url, output_path): | ||
| """Download a file to the specified path""" | ||
| try: | ||
| print(f" Downloading: {os.path.basename(output_path)}") |
There was a problem hiding this comment.
I'd suggest using the logging module instead of print statements globally: https://docs.python.org/3/library/logging.html
| response.raise_for_status() | ||
|
|
||
| create_dir_if_not_exists(output_dir) | ||
| items = response.json() |
There was a problem hiding this comment.
There was a problem hiding this comment.
So while this function does allow any URL to be parsed - which could cause this issue, the only URLs used by this function are specific locations within the repo, and not the whole repo. So unless we have 100+ rules, this shouldn't be an issue.
There was a problem hiding this comment.
This is indeed a potential issue. Considering the recursive situation, I have refactored the entire script to use git clone instead of Github APIs to download files. This approach is also more efficient than making individual calls.
| rules_dir = os.path.join(SPHINX_DIR, "styles/Canonical") | ||
| vocab_dir = os.path.join(SPHINX_DIR, "styles/config/vocabularies/Canonical") | ||
| dict_dir = os.path.join(SPHINX_DIR, "styles/config/dictionaries") | ||
|
|
||
| # GitHub API URLs | ||
| rules_github_url = f"{GITHUB_API_BASE}/contents/styles/Canonical" | ||
| vocab_github_url = f"{GITHUB_API_BASE}/contents/styles/config/vocabularies/Canonical" | ||
| dict_github_url = f"{GITHUB_API_BASE}/contents/styles/config/dictionaries" | ||
|
|
||
| # Download through GitHub API | ||
| if not download_github_directory(rules_github_url, rules_dir): | ||
| print(f"Failed to download directory from {rules_github_url}") | ||
| return 1 | ||
| if not download_github_directory(vocab_github_url, vocab_dir): | ||
| print(f"Failed to download directory from {vocab_github_url}") | ||
| return 1 | ||
| if not download_github_directory(dict_github_url, dict_dir): | ||
| print(f"Failed to download directory from {dict_github_url}") | ||
| return 1 |
There was a problem hiding this comment.
| rules_dir = os.path.join(SPHINX_DIR, "styles/Canonical") | |
| vocab_dir = os.path.join(SPHINX_DIR, "styles/config/vocabularies/Canonical") | |
| dict_dir = os.path.join(SPHINX_DIR, "styles/config/dictionaries") | |
| # GitHub API URLs | |
| rules_github_url = f"{GITHUB_API_BASE}/contents/styles/Canonical" | |
| vocab_github_url = f"{GITHUB_API_BASE}/contents/styles/config/vocabularies/Canonical" | |
| dict_github_url = f"{GITHUB_API_BASE}/contents/styles/config/dictionaries" | |
| # Download through GitHub API | |
| if not download_github_directory(rules_github_url, rules_dir): | |
| print(f"Failed to download directory from {rules_github_url}") | |
| return 1 | |
| if not download_github_directory(vocab_github_url, vocab_dir): | |
| print(f"Failed to download directory from {vocab_github_url}") | |
| return 1 | |
| if not download_github_directory(dict_github_url, dict_dir): | |
| print(f"Failed to download directory from {dict_github_url}") | |
| return 1 | |
| base_paths = [ | |
| "styles/Canonical", | |
| "styles/config/vocabularies/Canonical", | |
| "styles/config/dictionaries", | |
| ] | |
| downloads = [ | |
| ( | |
| f"{GITHUB_API_BASE}/contents/{base_path}", | |
| os.path.join(SPHINX_DIR, base_path), | |
| ) | |
| for base_path in base_paths | |
| ] | |
| failed_downloads = [ | |
| url | |
| for (url, destination) in downloads | |
| if not download_github_directory(url, destination) | |
| ] | |
| if failed_downloads: | |
| for url in failed_downloads: | |
| print(f"Failed to download directory from {url}") | |
| return 1 |
or something like this in case the layout is extended later
There was a problem hiding this comment.
Done - refactored into a dict
Change to use `git clone` to download the whole repository into /tmp, which is more efficient than API calls. Copy over the files once download is completed Add overwrite option
| # Create temporary directory on disk for cloning | ||
| temp_dir = tempfile.mkdtemp() | ||
| logging.info("Cloning repository <%s> to temporary directory: %s", GITHUB_REPO, temp_dir) | ||
| clone_cmd = ["git", "clone", "--depth", "1", GITHUB_CLONE_URL, temp_dir] |
There was a problem hiding this comment.
A note: for more intensive use, there's a nice wrapper: https://gitpython.readthedocs.io/en/stable/
There was a problem hiding this comment.
Thanks for sharing the link! It's a nice solution with pure python. But since we're only cloning one single repo here, I'll go with the original git command without installing another package
3 participants
(Resolves DOCPR-1416)
This pull request refactors the
get_vale_conf.pyscript to improve its structure and error handling.Description
* Modularize the code into new methodscreate_dir_if_not_exists,download_file, anddownload_github_directory.SPHINX_DIRinstead ofDIRto ensure consistency.* Improved error handling for timeout and API failure, with error messages for download failures.main()function to return appropriate exit codes when called by MakefileUpdate after review:
/tmpdirectory, and then copy the target files to the current project--no-overwriteto skip overwriting