-
Notifications
You must be signed in to change notification settings - Fork 93
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
refactor times.py #368
base: develop
Are you sure you want to change the base?
refactor times.py #368
Conversation
Review changes with SemanticDiff. Analyzed 1 of 1 files. Overall, the semantic diff is 1% smaller than the GitHub diff.
|
PR Summary
|
Preparing review... |
1 similar comment
Preparing review... |
6842f38
to
5243129
Compare
Preparing review... |
@@ -196,6 +199,7 @@ def __init__( | |||
**kwargs, | |||
) | |||
self.timestamp = timestamp | |||
self.when = when |
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.
when
and timestamp
should be mutually exclusive
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.
Hi @cleder,
Would you suggest I instead accept a parameter called pointInTime
(I'm open to an alternate name), which is of type KmlDateTime | When
so that users can only provide either a value of type When
or KmlDateTime
but not both?
@@ -206,6 +210,7 @@ def __repr__(self) -> str: | |||
f"id={self.id!r}, " | |||
f"target_id={self.target_id!r}, " | |||
f"timestamp={self.timestamp!r}, " | |||
f"when={self.when!r}, " |
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.
There is no need to add both when
and timestamp
, as the when
object can have more attributes like id
and targetId
it will be better to return just this.
---- | ||
precision (Optional[int]): The precision of the time values. | ||
verbosity (Verbosity): The verbosity level for the element. | ||
class When(_TimePrimitive): |
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.
I think When
, Begin
and End
should inherit from _BaseObject
not _TimePrimitive
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.
This should be fine as _BaseObject
is higher in the inheritance hierarchy while still having access to the class methods etree_element
and _get_kwargs
defined in the base class _XMLObject
.
- These two class methods will be needed to implement the
When
,Begin
andEnd
classes.
Resolves #359