Gerrrr commented on code in PR #131:
URL: https://github.com/apache/otava/pull/131#discussion_r3278530511
##########
otava/util.py:
##########
@@ -134,18 +134,47 @@ class DateFormatError(ValueError):
message: str
-def parse_datetime(date: Optional[str]) -> Optional[datetime]:
+def parse_datetime(date: Optional[Union[str, int, float, datetime]]) ->
Optional[datetime]:
"""
- Converts a human-readable string into a datetime object.
- Accepts many formats and many languages, see dateparser package.
- Raises DataFormatError if the input string format hasn't been recognized.
+ Normalize various datetime inputs into a timezone-aware datetime.
+
+ Supports:
+ - datetime (returned as-is)
+ - int/float (treated as Unix timestamp)
+ - str (parsed via dateparser)
+ - None (returns None)
"""
if date is None:
return None
- parsed: datetime = dateparser.parse(date,
settings={"RETURN_AS_TIMEZONE_AWARE": True})
- if parsed is None:
- raise DateFormatError(f"Invalid datetime value: {date}")
- return parsed
+
+ if isinstance(date, datetime):
+ return date
+
+ if isinstance(date, (int, float)):
+ return datetime.fromtimestamp(date, tz=timezone.utc)
+
+ if isinstance(date, str):
+ parsed = dateparser.parse(date, settings={"RETURN_AS_TIMEZONE_AWARE":
True})
+ if parsed is None:
+ raise DateFormatError(f"Invalid datetime value: {date}")
+ return parsed
+
+ raise TypeError(f"Unsupported type for datetime parsing: {type(date)}")
+
+
+def clean_str(value: Optional[str]) -> Optional[str]:
+ if value is None:
+ return None
+
+ if not isinstance(value, str):
+ return value # or raise TypeError if you want strictness
Review Comment:
This return violates the type signature. I'd throw a type error.
##########
otava/graphite.py:
##########
@@ -80,49 +79,32 @@ class GraphiteError(IOError):
@dataclass
class GraphiteEvent:
- test_owner: str
- test_name: str
- run_id: str
- status: str
- start_time: datetime
pub_time: datetime
- end_time: datetime
- version: Optional[str]
- branch: Optional[str]
- commit: Optional[str]
+ test_owner: Optional[str] = None
+ test_name: Optional[str] = None
+ run_id: Optional[str] = None
+ status: Optional[str] = None
+ start_time: Optional[datetime] = None
+ end_time: Optional[datetime] = None
+ version: Optional[str] = None
+ branch: Optional[str] = None
+ commit: Optional[str] = None
- def __init__(
- self,
- pub_time: int,
- test_owner: str,
- test_name: str,
- run_id: str,
- status: str,
- start_time: int,
- end_time: int,
- version: Optional[str],
- branch: Optional[str],
- commit: Optional[str],
- ):
- self.test_owner = test_owner
- self.test_name = test_name
- self.run_id = run_id
- self.status = status
- self.start_time = parse_datetime(str(start_time))
- self.pub_time = parse_datetime(str(pub_time))
- self.end_time = parse_datetime(str(end_time))
- if len(version) == 0 or version == "null":
- self.version = None
- else:
- self.version = version
- if len(branch) == 0 or branch == "null":
- self.branch = None
- else:
- self.branch = branch
- if len(commit) == 0 or commit == "null":
- self.commit = None
- else:
- self.commit = commit
+
+ def __post_init__(self):
+ if self.pub_time is None:
+ raise ValueError("pub_time is required and cannot be None")
+ # Ensure pub_time is always a datetime
+ self.pub_time = parse_datetime(str(self.pub_time))
Review Comment:
I think we should do `parse_datetime` on `start_time` and `date_time`. If
they are `None`, they will remain `None`. If they are strings/ints, they will
get converted to `datetime`. Without this change, we may get arbitrary types in
those fields. Example:
```python
def test_graphite_event_parses_start_and_end_time():
"""start_time and end_time are documented as supported tags and annotated
Optional[datetime]; if Graphite delivers them as Unix timestamps (int) or
ISO strings in the event data, they must be normalized to tz-aware
datetimes, the same way pub_time is."""
event = GraphiteEvent(
pub_time=1700000000,
start_time=1700000000,
end_time="2024-01-01 10:00:00",
)
assert isinstance(event.start_time, datetime)
assert event.start_time.tzinfo is not None
assert isinstance(event.end_time, datetime)
assert event.end_time.tzinfo is not None
```
##########
otava/graphite.py:
##########
@@ -80,49 +79,28 @@ class GraphiteError(IOError):
@dataclass
class GraphiteEvent:
- test_owner: str
- test_name: str
- run_id: str
- status: str
- start_time: datetime
pub_time: datetime
- end_time: datetime
- version: Optional[str]
- branch: Optional[str]
- commit: Optional[str]
-
- def __init__(
Review Comment:
This is fixed now, thanks!
##########
otava/graphite.py:
##########
@@ -80,49 +79,28 @@ class GraphiteError(IOError):
@dataclass
class GraphiteEvent:
- test_owner: str
- test_name: str
- run_id: str
- status: str
- start_time: datetime
pub_time: datetime
- end_time: datetime
- version: Optional[str]
- branch: Optional[str]
- commit: Optional[str]
-
- def __init__(
- self,
- pub_time: int,
- test_owner: str,
- test_name: str,
- run_id: str,
- status: str,
- start_time: int,
- end_time: int,
- version: Optional[str],
- branch: Optional[str],
- commit: Optional[str],
Review Comment:
I like this idea! Again, it is not required in this PR, but is worth doing
in a follow-up.
##########
otava/util.py:
##########
@@ -134,18 +134,47 @@ class DateFormatError(ValueError):
message: str
-def parse_datetime(date: Optional[str]) -> Optional[datetime]:
+def parse_datetime(date: Optional[Union[str, int, float, datetime]]) ->
Optional[datetime]:
"""
- Converts a human-readable string into a datetime object.
- Accepts many formats and many languages, see dateparser package.
- Raises DataFormatError if the input string format hasn't been recognized.
+ Normalize various datetime inputs into a timezone-aware datetime.
+
+ Supports:
+ - datetime (returned as-is)
+ - int/float (treated as Unix timestamp)
+ - str (parsed via dateparser)
+ - None (returns None)
"""
if date is None:
return None
- parsed: datetime = dateparser.parse(date,
settings={"RETURN_AS_TIMEZONE_AWARE": True})
- if parsed is None:
- raise DateFormatError(f"Invalid datetime value: {date}")
- return parsed
+
+ if isinstance(date, datetime):
+ return date
+
+ if isinstance(date, (int, float)):
+ return datetime.fromtimestamp(date, tz=timezone.utc)
+
+ if isinstance(date, str):
Review Comment:
This method supports strings, ints, floats, datetime now. However, all its
callers are using `str(input)`, so there are no callers at all that benefit
from this change.
While I am not asking to refactor callers of this method across the entire
codebase, do you think it would make sense to use it in Graphite importer? If
not... then what's the point of this change?
--
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]
