bito-code-review[bot] commented on code in PR #44514:
URL: https://github.com/apache/superset/pull/44514#discussion_r4078078861
##########
superset/tasks/deletion_retention.py:
##########
@@ -133,7 +133,11 @@ def _purge_impl(window_days: int, dry_run: bool) ->
dict[str, Any]:
# UTC-derived cutoff would shift the retention window by the server's
# timezone offset, purging early west of UTC. If deleted_at ever moves
# to UTC-aware, this must move with it.
- cutoff = datetime.now() - timedelta(days=window_days)
+ cutoff: datetime = (
+ datetime.now()
+ if window_days == -1
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Magic sentinel -1</b></div>
<div id="fix">
The `-1` 'immediate eligibility' sentinel is a bare literal repeated across
5 files (`cli`, `window.py`, `config.py`, both tasks). A named constant (e.g.
`RETENTION_IMMEDIATE = -1`) would encode the business meaning and keep the
guard/cutoff sites in lockstep. Consider defining it once and referencing it
everywhere.
</div>
</div>
<small><i>Code Review Run #ed9d1e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/config.py:
##########
@@ -1761,22 +1791,24 @@ def sync_theme_logo_href(
def _parse_version_history_retention_days() -> int:
"""Parse the retention window without making invalid input fatal."""
- value: str | None =
os.environ.get("SUPERSET_VERSION_HISTORY_RETENTION_DAYS")
+ value: str | None = os.environ.get("VERSION_HISTORY_RETENTION_DAYS")
if value is None:
return _DEFAULT_VERSION_HISTORY_RETENTION_DAYS
try:
- retention_days = int(value)
+ retention_days: int = int(value)
except ValueError:
logger.warning(
- "Invalid SUPERSET_VERSION_HISTORY_RETENTION_DAYS=%r; using %d",
+ "Invalid VERSION_HISTORY_RETENTION_DAYS=%r; using %d",
value,
_DEFAULT_VERSION_HISTORY_RETENTION_DAYS,
)
return _DEFAULT_VERSION_HISTORY_RETENTION_DAYS
+ if retention_days < -1:
+ logger.warning("Invalid negative VERSION_HISTORY_RETENTION_DAYS;
skipping")
+ return 0
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent invalid-negative handling</b></div>
<div id="fix">
`_parse_version_history_retention_days` returns 0 (disabled) for
`retention_days < -1`, silently stopping the prune, whereas
`_parse_soft_delete_retention_days` (line 1042) falls back to the default 30
for the same invalid input. A typo like `-2` (instead of `-1`) would silently
disable version-history pruning and let it grow unbounded. Align both parsers
on the same fallback.
</div>
</div>
<small><i>Code Review Run #ed9d1e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
