-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Filter away timestamps from diffs, increase testing. #199
Conversation
terjekv
commented
Jan 12, 2024
- This PR filters away timestamps from test results via a regexp.
- The timestamps are replaced inline so other content in the same value are retained.
- Adds testing of object history when supported (host, group, atom, role).
- Moves filtering testing to a host.
- Will conflict with Prevent group history from iterating over None. #198, due to having to fix group history here as well.
|
||
|
||
def group_objects(json_file_path): | ||
def replace_timestamps(obj: Any) -> Any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only consider implementing this if you think replace_timestamps()
could actually be called with typed values in the future (currently only Any
is passed to it).
def replace_timestamps(obj: Any) -> Any: | |
from typing import TypeVar | |
TSType = TypeVar("TSType", str, list, dict) | |
def replace_timestamps(obj: TSType) -> TSType: |
This is not perfect, but provides some very rudimentary type checking:
reveal_type(replace_timestamps("2020-01-01T00:00:00.000Z"))
reveal_type(replace_timestamps(["2020-01-01T00:00:00.000Z", "2020-01-01T00:00:00.000Z"]))
reveal_type(replace_timestamps({"foo": "2020-01-01T00:00:00.000Z"}))
% mypy ci
ci/diff.py:27: note: Revealed type is "builtins.str"
ci/diff.py:28: note: Revealed type is "builtins.list[Any]"
ci/diff.py:29: note: Revealed type is "builtins.dict[Any, Any]"
With a more complicated TypeVar
we could get rid of the Any
from the returned list
and dict
type annotations, but that's overkill for something that isn't even part of the CLI itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
- This PR filters away timestamps from test results via a regexp. - The timestamps are replaced inline so other content in the same value are retained. - Adds testing of object history when supported (host, group, atom, role). - Moves filtering testing to a host. - Will conflict with unioslo#198, due to having to fix group history here as well.
92c88f2
to
99b8c4d
Compare
If we're going the typing route, we should probably use a date library and properly parse things, but I'm not sure I see the huge value in this context. |