Skip to content

Commit 8cccbc4

Browse files
fix: resolve comprehensive codebase review findings
- docs: update CONTRIBUTING.md lint task descriptions - build: fix pyproject.toml per-file-ignores glob for tests - ui: replace duplicate error-msg IDs with form-error-text class - core: use exact line matching for duplicate author check - core: add exponential backoff for pygit2.clone_repository - i18n: fix Italian translation for 'Quit' - core: add dict type guard for markdown yaml metadata - ui: sanitize authentication error messages in LoadingScreen - ui: add XOR validation for social platform/URL entries - ui: restore focus indicator for Select widget in styles - ui: sort markdown files for deterministic order in member list - refactor: add explicit type annotations to MemberApp variables - test: fix various unit and E2E test failures and edge cases
1 parent 1fed7d4 commit 8cccbc4

21 files changed

Lines changed: 94 additions & 39 deletions

CONTRIBUTING.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ This document explains how to contribute to the edit-python.pe project, the comm
2929

3030
We use `poethepoet` (poe) as our task runner. Run the following commands as needed during development:
3131

32-
- `uv run poe lint`: Runs the `ruff` linter (and checks types using `ty`).
32+
- `uv run poe lint`: Runs the `ruff` linter.
3333
- `uv run poe lint:format`: Formats the code using `ruff format`.
3434
- `uv run poe lint:types`: Checks types using `ty check`.
3535
- `uv run poe lint:docs`: Lints Markdown files using `markdownlint` (excludes `CHANGELOG.md`).

pyproject.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ ignore = [
141141
]
142142

143143
[tool.ruff.lint.per-file-ignores]
144-
"tests/*" = [
144+
"tests/**/*.py" = [
145145
"S101", # Use of assert detected
146146
"S105", # Use of hardcoded password string
147147
"S106", # Use of hardcoded password function args

src/edit_python_pe/app.py

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
from github.Repository import Repository
12
from textual.app import App
23

34
from .screens.language import LanguageScreen
@@ -8,10 +9,10 @@ class MemberApp(App):
89

910
def __init__(self) -> None:
1011
super().__init__()
11-
self.original_repo = None
12-
self.forked_repo = None
13-
self.token = ""
14-
self.repo_path = ""
12+
self.original_repo: Repository | None = None
13+
self.forked_repo: Repository | None = None
14+
self.token: str | None = ""
15+
self.repo_path: str | None = ""
1516

1617
def on_mount(self) -> None:
1718
self.push_screen(LanguageScreen())

src/edit_python_pe/components/form_control.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,21 +26,21 @@ def compose(self) -> ComposeResult:
2626
if self.help_text_content:
2727
yield Static(self.help_text_content, classes="form-help-text")
2828

29-
yield Static("", classes="form-error-text", id="error-msg")
29+
yield Static("", classes="form-error-text")
3030

3131
def on_mount(self) -> None:
32-
self.query_one("#error-msg", Static).display = False
32+
self.query_one(".form-error-text", Static).display = False
3333

3434
def show_error(self, message: str) -> None:
35-
error_static = self.query_one("#error-msg", Static)
35+
error_static = self.query_one(".form-error-text", Static)
3636
error_static.update(message)
3737
error_static.display = True
3838

3939
# Apply has-error class to self
4040
self.add_class("has-error")
4141

4242
def clear_error(self) -> None:
43-
error_static = self.query_one("#error-msg", Static)
43+
error_static = self.query_one(".form-error-text", Static)
4444
error_static.update("")
4545
error_static.display = False
4646

src/edit_python_pe/file_io.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ def _write_authors_file(
3434
contents = ""
3535

3636
alias = aliases[0] if aliases else name
37-
file_content = f"\n{name}({alias}) <{email}>"
38-
if file_content.strip() not in contents:
39-
_append_file(file_content, file_path)
37+
candidate = f"{name}({alias}) <{email}>"
38+
normalized_lines = [line.strip() for line in contents.splitlines()]
39+
if candidate not in normalized_lines:
40+
_append_file(f"\n{candidate}", file_path)

src/edit_python_pe/github_client.py

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1+
import logging
12
import os
3+
import random
24
import shutil
35
from time import sleep
46

@@ -13,6 +15,8 @@
1315
from .markdown_builder import _create_member_file
1416
from .strings import _
1517

18+
logger = logging.getLogger(__name__)
19+
1620

1721
def get_repo(token: str) -> tuple[str, Repository]:
1822
auth = Auth.Token(token)
@@ -41,8 +45,27 @@ def fork_repo(token: str, original_repo: Repository) -> tuple[str, Repository]:
4145
callbacks = pygit2.callbacks.RemoteCallbacks(
4246
credentials=pygit2.UserPass(token, "x-oauth-basic")
4347
)
44-
sleep(3)
45-
pygit2.clone_repository(forked_repo_url, repo_path, callbacks=callbacks)
48+
49+
max_retries = 5
50+
for attempt in range(max_retries):
51+
try:
52+
pygit2.clone_repository(forked_repo_url, repo_path, callbacks=callbacks)
53+
break
54+
except Exception as e:
55+
if attempt == max_retries - 1:
56+
logger.error(
57+
"Failed to clone forked repository after %d attempts", max_retries
58+
)
59+
raise
60+
sleep_time = 2**attempt + random.uniform(0, 1) # noqa: S311
61+
logger.warning(
62+
"Attempt %d to clone repository failed: %s. Retrying in %.2fs...",
63+
attempt + 1,
64+
e,
65+
sleep_time,
66+
)
67+
sleep(sleep_time)
68+
4669
return repo_path, forked_repo
4770

4871

@@ -57,7 +80,7 @@ def create_pr(
5780
name: str,
5881
email: str,
5982
) -> tuple[str, str | None]:
60-
name_file, file_path = _create_member_file(
83+
name_file, _unused_file_path = _create_member_file(
6184
file_content,
6285
current_file,
6386
repo_path,
-4 Bytes
Binary file not shown.

src/edit_python_pe/locale/it/LC_MESSAGES/messages.po

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ msgstr "Indietro"
9494
#: src/edit_python_pe/screens/member_list.py:30
9595
#: src/edit_python_pe/screens/save_loading.py:53
9696
msgid "Quit"
97-
msgstr "Esentato"
97+
msgstr "Esci"
9898

9999
#: src/edit_python_pe/screens/dashboard.py:20
100100
msgid "Add New Member"

src/edit_python_pe/markdown_builder.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,9 @@ def load_file_into_form(
8484
except Exception:
8585
yaml_data = {}
8686

87+
if not isinstance(yaml_data, dict):
88+
yaml_data = {}
89+
8790
screen.name_input.value = yaml_data.get("author", "")
8891
screen.city_input.value = yaml_data.get("location", "")
8992

src/edit_python_pe/screens/loading.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import logging
12
from typing import TYPE_CHECKING, cast
23

34
from textual import work
@@ -15,6 +16,8 @@
1516
from .dashboard import DashboardScreen
1617
from .quit_confirm import QuitConfirmScreen
1718

19+
logger = logging.getLogger(__name__)
20+
1821

1922
class LoadingScreen(Screen):
2023
def __init__(self, token: str, **kwargs) -> None:
@@ -58,7 +61,7 @@ def check_quit(quit_app: bool | None) -> None:
5861
@work(thread=True, exclusive=True)
5962
def authenticate_and_clone(self) -> None:
6063
try:
61-
_, original_repo = get_repo(self.token)
64+
_token, original_repo = get_repo(self.token)
6265
repo_path, forked_repo = fork_repo(self.token, original_repo)
6366

6467
app = cast("MemberApp", self.app)
@@ -69,8 +72,12 @@ def authenticate_and_clone(self) -> None:
6972
app.token = self.token
7073

7174
self.app.call_from_thread(self.show_success)
72-
except Exception as e:
73-
error_message = str(e)
75+
except Exception:
76+
logger.error("Error during authentication and cloning", exc_info=True)
77+
error_message = _(
78+
"An unexpected error occurred while loading. "
79+
"Please try again or contact support."
80+
)
7481
self.app.call_from_thread(self.show_error, error_message)
7582

7683
def show_success(self) -> None:

0 commit comments

Comments
 (0)