AbsaOSS / AbsaOSS/EventGate

Remove __all__ declarations from src/utils modules

Ouverte
#221 0 commentaires 0 réactions 0 personnes assignées Voir sur GitHub
refactoring type:tech-debt
Langage dominant
Python
Étoiles
4
Forks
0
Merge moyen
1 j 5 h
PR mergées (30 j)
8

Description

### Description of Technical Debt

Five modules under `src/utils/` end with an `__all__` declaration:

| Module | Line | Names |
|---|---|---|
| `src/utils/conf_path.py` | 90 | 4 |
| `src/utils/logging_levels.py` | 71 | 3 |
| `src/utils/observability.py` | 217 | 16 |
| `src/utils/safe_serialization.py` | 87 | 1 |
| `src/utils/trace_logging.py` | 53 | 1 |

`__all__` controls exactly one thing: which names `from module import *` binds. There is no `import *` anywhere in `src/` or `tests/`, and explicit imports are the convention the codebase already follows everywhere. The declarations therefore have no effect on any current call site.

Raised in review on PR #204 and agreed by both reviewer and author.

### Impact of Technical Debt

- **Maintenance with no return.** Every new public name in these modules means editing a second list that nothing reads. The lists are in sync today, so the cost so far has been paid entirely in review attention rather than caught bugs.
- **Misleading signal.** A reader encountering `__all__` reasonably infers that star imports are expected somewhere, or that the module has a curated public surface distinct from its non-underscore names. Neither is true.
- **Inconsistent with the rest of the codebase.** No other package in the project declares `__all__`, and it is not a pattern used in sibling Python projects, so it reads as local convention where there is none.

### Category

Code Quality / Refactoring

### Priority

Low - Nice to have

### Proposed Solution

Delete the `__all__` block from all five modules. Rely on the existing convention: a leading underscore marks a name private, everything else is importable.

Two things to confirm while doing it, both currently safe:

1. **Re-exports.** `src/utils/observability.py` lists `TRACE_LEVEL` and `configured_log_level`, which it imports from `src/utils/logging_levels.py` rather than defining; `src/utils/conf_path.py` similarly lists `CONF_DIR` and `INVALID_CONF_ENV`. Under mypy's `no_implicit_reexport` an imported name is only re-exported if it appears in `__all__` or is aliased. The project's mypy config (`pyproject.toml [tool.mypy]`) does **not** enable `strict` or `no_implicit_reexport`, so removal changes nothing — but if that config is tightened later, importers of those names through `observability` would need to import them from `logging_levels` instead.
2. **No star imports.** `grep -rn "import \*" --include=*.py src/ tests/` returns nothing. Re-run before merging in case one has been added.

### Effort Estimate

< 1 hour

### Dependencies / Related

- PR #204, where this was raised
- #193

### Additional Context

Acceptance:

- No `__all__` remains under `src/`.
- `pytest tests/unit`, `mypy`, `pylint`, and `black --check` all pass unchanged.
- No import statement anywhere needs editing, which is the point: if one does, that name was relying on a re-export and should be imported from its defining module.

Guide de contribution

Aucun guide de contribution indexé pour ce dépôt

Évaluation

Cette issue n'a pas encore été évaluée.

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.