refactor/ create architecture for package - #62
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces a new modular repo_exporter package architecture that consolidates shared logic between the GitHub and Hugging Face exporters into a common base class and provides a unified CLI entry point.
Changes:
- Added
BaseExporterwith shared utilities, Google Sheets write helpers, and commonrun()orchestration. - Implemented
GitHubExporterandHuggingFaceExportersubclasses for platform-specific fetching/metadata + sheet formatting. - Added a CLI (
main.py) and package entry points (__init__.py,__about__.py) for external imports and versioning.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/repo_exporter/base.py | Adds shared exporter base class, Sheets helpers, and run orchestration. |
| src/repo_exporter/github.py | Adds GitHub-specific exporter implementation and metadata extraction. |
| src/repo_exporter/huggingface.py | Adds Hugging Face-specific exporter implementation and metadata extraction. |
| src/repo_exporter/main.py | Adds CLI entry point and export_repos() dispatcher. |
| src/repo_exporter/init.py | Exposes exporters and __version__ as the package API surface. |
| src/repo_exporter/about.py | Defines the package version string. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
egrace479
left a comment
There was a problem hiding this comment.
This is a HUGE PR, and you've done great. Preliminary feedback on the structuring.
Co-authored-by: Elizabeth Campolongo <38985481+egrace479@users.noreply.github.com>
| def get_column_index(self, header:list, col_name: str): | ||
| try: | ||
| return header.index(col_name) | ||
| except ValueError: | ||
| return None |
| try: | ||
| if repo.get_readme(): | ||
| return "Yes" | ||
| except GithubException: | ||
| return "No" | ||
|
|
| try: | ||
| if repo.get_license(): | ||
| return "Yes" | ||
| except GithubException: | ||
| return "No" | ||
|
|
| rules = [] | ||
| for col_set, color in [ | ||
| (red_columns, {"red": 1, "green": 0.5, "blue": 0.5}), | ||
| (secondary_columns, secondary_color), | ||
| ]: | ||
| for col_name in col_set: | ||
| col_index = self.get_column_index(header, col_name) | ||
| if col_index is None: | ||
| continue | ||
| rules.append({ | ||
| "addConditionalFormatRule": { | ||
| "rule": { | ||
| "ranges": [{ | ||
| "sheetId": sheet.id, | ||
| "startRowIndex": HEADER_ROW_INDEX, | ||
| "endRowIndex": HEADER_ROW_INDEX + len(df), | ||
| "startColumnIndex": col_index, | ||
| "endColumnIndex": col_index + 1, | ||
| }], | ||
| "booleanRule": { | ||
| "condition": { | ||
| "type": "TEXT_EQ", | ||
| "values": [{"userEnteredValue": "No"}], | ||
| }, | ||
| "format": {"backgroundColor": color}, | ||
| }, | ||
| }, | ||
| "index": 0, | ||
| } | ||
| }) | ||
|
|
||
| sheet.spreadsheet.batch_update({"requests": rules}) |
There was a problem hiding this comment.
@copilot, please don't commit anything but suggest a fix.
There was a problem hiding this comment.
Here's a suggested fix that deletes all existing conditional format rules for the sheet before adding new ones. This prevents rule accumulation across runs.
def _apply_conditional_formatting(
self,
sheet,
header: list[str],
df: pd.DataFrame,
red_columns: set[str],
secondary_columns: set[str],
secondary_color: dict,
) -> None:
HEADER_ROW_INDEX = 2
spreadsheet = sheet.spreadsheet
sheet_id = sheet.id
# Delete all existing conditional format rules for this sheet to prevent accumulation
existing_rules = spreadsheet.fetch_sheet_metadata()
sheets_meta = existing_rules.get("sheets", [])
delete_requests = []
for s in sheets_meta:
if s["properties"]["sheetId"] == sheet_id:
num_rules = len(s.get("conditionalFormats", []))
# Delete from last to first to keep indices stable
for i in range(num_rules - 1, -1, -1):
delete_requests.append({
"deleteConditionalFormatRule": {
"sheetId": sheet_id,
"index": i,
}
})
break
# Build new rules
new_rules = []
for col_set, color in [
(red_columns, {"red": 1, "green": 0.5, "blue": 0.5}),
(secondary_columns, secondary_color),
]:
for col_name in col_set:
col_index = self.get_column_index(header, col_name)
if col_index is None:
continue
new_rules.append({
"addConditionalFormatRule": {
"rule": {
"ranges": [{
"sheetId": sheet_id,
"startRowIndex": HEADER_ROW_INDEX,
"endRowIndex": HEADER_ROW_INDEX + len(df),
"startColumnIndex": col_index,
"endColumnIndex": col_index + 1,
}],
"booleanRule": {
"condition": {
"type": "TEXT_EQ",
"values": [{"userEnteredValue": "No"}],
},
"format": {"backgroundColor": color},
},
},
"index": 0,
}
})
all_requests = delete_requests + new_rules
if not all_requests:
return
spreadsheet.batch_update({"requests": all_requests})The key change: before adding rules, we fetch the sheet metadata, count existing conditional format rules on the target sheet, and issue deleteConditionalFormatRule requests (from last index to first to keep indices stable). Then the deletes and new adds are sent in a single batch_update call.
There was a problem hiding this comment.
Is it possible to efficiently address this through checking for existing rules? @mandylan75, see the response here.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…m/Imageomics/repo-exporter into feature/issue-58/consolidate-code
Co-authored-by: Elizabeth Campolongo <38985481+egrace479@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/repo_exporter/base.py:318
- _apply_conditional_formatting() currently indexes all existing conditional formats by startColumnIndex, and then deletes any rules whose column isn't in
desired(lines 308-347). That can unintentionally delete user- or manually-created conditional formatting rules on the sheet that have nothing to do with the exporter.
Consider only managing rules that match the exporter’s own signature (TEXT_EQ == "No" and the expected startRowIndex), and ignore all other conditional formats so they’re left untouched.
existing_rules = {} # col_index -> (rule_index, rule_dict)
metadata = sheet.spreadsheet.fetch_sheet_metadata()
for sheet_meta in metadata.get("sheets", []):
if sheet_meta["properties"]["sheetId"] != sheet_id:
continue
for i, rule in enumerate(sheet_meta.get("conditionalFormats", [])):
ranges = rule.get("ranges", [{}])
if not ranges:
continue
existing_rules[ranges[0].get("startColumnIndex")] = (i, rule)
break
requests = []
# 1. Updates first; index safe since nothing has shifted yet.
for col_index, color in desired.items():
if col_index not in existing_rules:
continue
rule_index, existing_rule = existing_rules[col_index]
existing_range = existing_rule.get("ranges", [{}])[0]
existing_color = (
existing_rule.get("booleanRule", {}).get("format", {}).get("backgroundColor", {})
)
if existing_range.get("endRowIndex") != end_row or self._normalize_color(existing_color) != self._normalize_color(color):
requests.append({
"updateConditionalFormatRule": {
"index": rule_index,
"sheetId": sheet_id,
"rule": self._build_conditional_rule(sheet_id, col_index, end_row, color),
}
})
# 2. Deletes next, highest index first; deleting descending never
# shifts the index of a rule we still need to delete
stale_indices = sorted(
(rule_index for col_index, (rule_index, _) in existing_rules.items() if col_index not in desired),
reverse=True,
)
for rule_index in stale_indices:
requests.append({"deleteConditionalFormatRule": {"sheetId": sheet_id, "index": rule_index}})
test_normalize.py:13
- This file looks like a one-off debug script (prints to stdout) rather than a test or library module. Since it lives at the repo root it’s easy to confuse with real tests/code, but it won’t be executed by pytest (testpaths is
tests). Consider removing it from the package PR or moving the relevant assertion into an actual pytest test (e.g.tests/repo_exporter/test_base.py).
from repo_exporter.base import BaseExporter
# Simulate the API omitting zero-valued channels
api_response = {"red": 1} # green/blue omitted because they're 0
literal = {"red": 1, "green": 0.5, "blue": 0.5}
print(BaseExporter._normalize_color(api_response))
print(BaseExporter._normalize_color(literal))
print(BaseExporter._normalize_color(api_response) == BaseExporter._normalize_color(literal))
same_full = {"red": 1, "green": 0.5, "blue": 0.5}
same_partial = {"red": 1, "green": 0.5, "blue": 0.5}
print(BaseExporter._normalize_color(same_full) == BaseExporter._normalize_color(same_partial)) # should be True
test_new_column_logic.py:114
- This appears to be an ad-hoc test harness for the legacy
gh_repo_exporter.update_google_sheetfunction (and includes__main__prints). Since it’s at the repo root it won’t run under the configured pytesttestpaths = ["tests"], and it also couples this refactor PR back to the legacy script.
Consider either removing it, or converting it into a real pytest module under tests/ that targets the new BaseExporter._sync_new_columns behavior instead.
import pandas as pd
from unittest.mock import MagicMock, patch, call
from gh_repo_exporter import update_google_sheet
def _make_mock_sheet(fake_header, fake_data_rows):
mock_sheet = MagicMock()
mock_sheet.title = "GH-Repos"
mock_sheet.id = 0
mock_sheet.row_values.return_value = fake_header
mock_sheet.get_all_values.return_value = [[], fake_header] + fake_data_rows
mock_spreadsheet = MagicMock()
mock_spreadsheet.worksheet.return_value = mock_sheet
mock_sheet.spreadsheet = mock_spreadsheet
mock_client = MagicMock()
mock_client.open_by_key.return_value = mock_spreadsheet
return mock_sheet, mock_spreadsheet, mock_client
def _run(df, mock_client):
with patch("gh_repo_exporter.gspread.authorize", return_value=mock_client), \
patch("gh_repo_exporter.Credentials.from_service_account_file", return_value=MagicMock()):
update_google_sheet(df, "fake_spreadsheet_id", "GH-Repos", "fake_creds.json")
def test_new_column_is_appended():
"""Existing behavior: a DataFrame column not in the sheet header gets appended."""
fake_header = ["Repository Name", "Stars", "README"]
fake_data_rows = [["=HYPERLINK(\"url\", \"cool-project\")", "10", "Yes"]]
mock_sheet, mock_spreadsheet, mock_client = _make_mock_sheet(fake_header, fake_data_rows)
df = pd.DataFrame([{
"Repository Name": '=HYPERLINK("url", "cool-project")',
"Stars": 10,
"README": "Yes",
"TEST_COLUMN": "test123",
}])
_run(df, mock_client)
assert mock_sheet.update.call_args == call(range_name="D2", values=[["TEST_COLUMN"]])
print("PASS: test_new_column_is_appended")
def test_renamed_column_creates_duplicate_not_inplace_update():
"""Documents current behavior: renaming a column (e.g. 'Language' -> 'Primary Language')
does NOT update the existing 'Language' header in place. Instead the new name is treated
as a brand-new column and appended, leaving the old header untouched.
"""
fake_header = ["Repository Name", "Language"]
fake_data_rows = [["=HYPERLINK(\"url\", \"cool-project\")", "Python"]]
mock_sheet, mock_spreadsheet, mock_client = _make_mock_sheet(fake_header, fake_data_rows)
df = pd.DataFrame([{
"Repository Name": '=HYPERLINK("url", "cool-project")',
"Primary Language": "Python",
}])
_run(df, mock_client)
# "Primary Language" gets appended as a new column at index 3 (C2), NOT written into
# the existing "Language" column at index 2 (B2).
assert mock_sheet.update.call_args == call(range_name="C2", values=[["Primary Language"]])
print("PASS: test_renamed_column_creates_duplicate_not_inplace_update (documents current behavior)")
def test_no_new_columns_skips_header_update():
"""When every DataFrame column already exists in the sheet header, sheet.update()
(the header-writing call) should never be called."""
fake_header = ["Repository Name", "Stars", "README"]
fake_data_rows = [["=HYPERLINK(\"url\", \"cool-project\")", "10", "Yes"]]
mock_sheet, mock_spreadsheet, mock_client = _make_mock_sheet(fake_header, fake_data_rows)
df = pd.DataFrame([{
"Repository Name": '=HYPERLINK("url", "cool-project")',
"Stars": 20,
"README": "No",
}])
_run(df, mock_client)
mock_sheet.update.assert_not_called()
print("PASS: test_no_new_columns_skips_header_update")
def test_multiple_new_columns_appended_together():
"""Two new columns at once should be appended starting at the correct index,
in the same order they appear in df.columns."""
fake_header = ["Repository Name", "Stars"]
fake_data_rows = [["=HYPERLINK(\"url\", \"cool-project\")", "10"]]
mock_sheet, mock_spreadsheet, mock_client = _make_mock_sheet(fake_header, fake_data_rows)
df = pd.DataFrame([{
"Repository Name": '=HYPERLINK("url", "cool-project")',
"Stars": 10,
"Model": "No",
"Dataset": "No",
}])
_run(df, mock_client)
assert mock_sheet.update.call_args == call(range_name="C2", values=[["Model", "Dataset"]])
print("PASS: test_multiple_new_columns_appended_together")
if __name__ == "__main__":
test_new_column_is_appended()
test_renamed_column_creates_duplicate_not_inplace_update()
test_no_new_columns_skips_header_update()
test_multiple_new_columns_appended_together()
…nditional formatting
…m/Imageomics/repo-exporter into feature/issue-58/consolidate-code
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/repo_exporter/huggingface.py:166
- get_card_field() only reads repo.card_data; the legacy exporter uses repo.cardData (and HF objects may expose only that attribute). If card_data is missing, this will fall into the exception path and return "N/A" even when cardData contains the requested fields (license/description). Consider supporting both attribute names.
try:
for key in keys:
value = repo.card_data.get(key, "")
if value:
src/repo_exporter/github.py:288
- get_top_contributors() sleeps 20s between retries whenever get_stats_contributors() returns a falsy value, which can add 40s per repo even though a commit-based fallback exists. This can make large org exports take hours. Consider only waiting on the specific "202 stats are being generated" response and using a shorter backoff.
for _ in range(3):
stats = repo.get_stats_contributors()
if stats:
break
time.sleep(20)
src/repo_exporter/main.py:121
- The --repo-type help text advertises the default as GH_REPO_TYPE (often "None"), but export_repos() actually defaults to "all" when GH_REPO_TYPE is unset. Updating the help keeps CLI behavior and documentation aligned.
help=f"Repo type filter: all, public, private, forks, sources, member "
f"(overrides GH_REPO_TYPE in .env; default: {GH_REPO_TYPE})"
beanbean9339
left a comment
There was a problem hiding this comment.
Approved! Solid refactor with thorough test coverage across the Hugging Face exporter, CLI, edge cases, and error handling. No blocking issues from me. Nice work! 👍
| """ | ||
| ... | ||
|
|
||
| @abstractmethod |
There was a problem hiding this comment.
I agree with making red_columns and secondary_columns abstract properties. update_google_sheet() depends on them, so this makes the subclass contract explicit and keeps the architecture consistent with the other abstract methods.
|
|
||
| repo = NoCitationRepo() | ||
| readme = '[](https://zenodo.org/badge/latestdoi/195575274)' | ||
| assert has_doi(repo, readme) == "https://zenodo.org/badge/latestdoi/195575274" |
There was a problem hiding this comment.
Nice coverage of the fallback behavior. Could we add a case where CITATION.cff contains an invalid DOI but the README has a valid Zenodo badge? That would verify the fallback works not only when the DOI is absent, but also when it fails validation.
There was a problem hiding this comment.
Great idea, I will add that that to the tests!
There was a problem hiding this comment.
Actually, re-reading this, I don't think we would want that to count. We don't want repos with citations that have invalid DOIs, since they would then be cited incorrectly.
| """ | ||
| repo = FakeRepo(citation) | ||
| assert is_valid_doi("10.9999/zenodo.10000000") is True | ||
| assert has_doi(repo) == "https://doi.org/10.9999/zenodo.10000000" |
There was a problem hiding this comment.
Could we add a case with multiple entries in identifiers, including a non-DOI entry before the valid DOI? This would verify that unrelated identifiers are skipped correctly.
There was a problem hiding this comment.
Yes, this shouldn't be a problem since i created functions that made it so that specifically the zenodo DOI gets checked by its format, but I will add that to the tests as well.
There was a problem hiding this comment.
I think test_identifiers_type_doi is sufficient here since it tests the standard identifiers structure in CITATION.cff. I'd drop test_identifiers_doi unless the shorthand format is intentionally supported.
There was a problem hiding this comment.
One thing worth confirming separately is the documented License == "N/A" behavior for repos without license metadata, since the test notes that this differs from the legacy exporter. Otherwise, I don't see any blocking issues.
There was a problem hiding this comment.
Thank you for the feedback! I will consider adding these suggestions into a separate PR since this PR desperately needs to be approved but I will for sure come back to it!
There was a problem hiding this comment.
I've configured the .env locally, and was able to export huggingface repos to the googlesheet successfully. The github repo export takes much longer, and it paused at around 90% for a long time with no warnings or time-out errors. I'll try it again this afternoon.
The README wasn't updated for the refactor, it's unchanged from main and still documents the deleted scripts.
python gh_repo_exporter.py / python hf_repo_exporter.py no longer work on this branch. They should be replaced with the new pkg entry point commands. And the new repo-exporter github|huggingface CLI with its flags (--org, --repo-type, --spreadsheet-id, …) should be documented as well.
NetZissou
left a comment
There was a problem hiding this comment.
.github/workflows/gh-repo-exporter.yml
.github/workflows/hf-repo-exporter.yml
Both workflow files still run the deleted scripts (python gh_repo_exporter.py / python hf_repo_exporter.py + pip install -r requirements.txt). They should be updated and schedule a re-run to make sure it can be executable.
|
And just a follow-up on my previous comment, the GitHub repo export executed successfully in the second attempt. The features are functioning well. |
| repo_type - String. Repo type. | ||
| """ | ||
| try: | ||
| commits = self.api.list_repo_commits(repo_id=repo_id, repo_type=repo_type) |
There was a problem hiding this comment.
This should probably be listed in a separate issue, similarly to get_top_contributors()
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
egrace479
left a comment
There was a problem hiding this comment.
A couple small notes on presentation
| # Repository Exporter [](https://doi.org/10.5281/zenodo.17835081) | ||
|
|
||
| Python scripts that gather metadata for all repositories in a provided GitHub or Hugging Face organization and automatically export the data into a desired Google Sheet (using a Google Cloud Console Service Account) for easy viewing and analysis. | ||
| Python package that gathers metadata for all repositories in a provided GitHub or Hugging Face organization and automatically exports the data into a desired Google Sheet (using a Google Cloud Console Service Account) for easy viewing and analysis. |
There was a problem hiding this comment.
| Python package that gathers metadata for all repositories in a provided GitHub or Hugging Face organization and automatically exports the data into a desired Google Sheet (using a Google Cloud Console Service Account) for easy viewing and analysis. | |
| Python package that gathers metadata for all repositories in a provided GitHub or Hugging Face organization and automatically exports the data into a designated Google Sheet (using a Google Cloud Console Service Account) for easy viewing and analysis. |
|
|
||
| | Flag | Overrides | Description | | ||
| |---|---|---| | ||
| | `--org` | `HF_ORG_NAME` | Hugging Face organization name (case-sensitive) | |
There was a problem hiding this comment.
| | `--org` | `HF_ORG_NAME` | Hugging Face organization name (case-sensitive) | | |
| | `--org` | `HF_ORG_NAME` | Hugging Face organization name (**case-sensitive**) | |
| - **Run both exporters (wait for one to finish before running the other)** | ||
| ``` | ||
| python hf_repo_exporter.py | ||
| python gh_repo_exporter.py | ||
| repo-exporter huggingface | ||
| repo-exporter github | ||
| ``` | ||
|
|
There was a problem hiding this comment.
I think we could replace this with a simple statement that they can be run sequentially; each must be run individually. (I can't directly make the suggestion)
Do they have to be run one at a time? As in, is there any reason I couldn't run the commands one after the other before it's finished?
There was a problem hiding this comment.
Great catch! I double checked with the code and there's no dependency between them, so they can both be run individually since they're separate processes.
#58
__about__.py: Package version string (version = "2.0.0").__init__.py: Package entry point, exposesGitHubExporter,HuggingFaceExporter, and__version__for external imports.base.py: BaseExporter abstract base class. Holds all logic shared between GitHub and Hugging Face: shared utilities (is_inactive,extract_display_name,ensure_string_value), Google Sheets helpers (_get_sheet,_build_batch_body,_write_batch,_apply_conditional_formatting), and therun()method that both platform exporters inherit. Sheets logic lives directly inbase.pyrather than a separatesheets.py, since the helpers are tightly coupled toBaseExporter's state.github.py: GitHubExporter(BaseExporter). GitHub-specific repo fetching and metadata logic (contributors, DOI, license, package requirement files, website/paper/dataset/model detection from README) and GH-specific sheet column/color config.huggingface.py: HuggingFaceExporter(BaseExporter). Hugging Face-specific repo fetching (models/datasets/spaces) and metadata logic (author, contributors, license, DOI, associated models/spaces, README field extraction) plus HF-specific sheet column/color config.__main__.py: CLI entry point with argparse support (--platform, --token, --org, --spreadsheet-id, --sheet-name, --credentials-path, --repo-type); builds and runs the appropriate exporter based on --platform, with CLI args overriding .env values.