refactor(system): add historian.types submodule#28
Merged
cesarcoatl merged 2 commits into8.3from Apr 28, 2026
Merged
Conversation
Contributor
Reviewer's GuideIntroduces a new historian.types system-level helper module and corresponding Java historian data model stubs, while bumping the java-api dependency to expose those model types and wiring new system.historian types stubs into the public API surface. Class diagram for new historian data model typesclassDiagram
class SnapshotCapable {
+bool isSnapshot()
+Instant snapshotTime()
+SnapshotCapable withSnapshot(Instant timestamp)
}
class TemporalPoint {
+int compareTo(TemporalPoint other)
+QualifiedPath source()
+Instant timestamp()
+Any type()
+Any value()
}
class DataPoint {
+QualityCode quality()
+Any valueClass()
}
class AtomicPoint {
}
class Annotation {
+Annotation(notes, type_, author)
+str author()
+str notes()
+str type()
}
class AnnotationPoint {
+AnnotationPoint(identifier, source, value, startTime, endTime, lastUpdated)
+static Builder builder()
+Instant endTime()
+UUID identifier()
+Instant lastUpdated()
+QualifiedPath source()
+Instant startTime()
+Instant timestamp()
+StandardComplexPointType type()
+Annotation value()
+AnnotationPoint withSource(QualifiedPath source)
}
class AnnotationPoint_Builder {
+AnnotationPoint_Builder()
+AnnotationPoint_Builder annotationType(str annotationType)
+AnnotationPoint_Builder author(str author)
+AnnotationPoint build()
+AnnotationPoint_Builder endTime(Instant endTime)
+AnnotationPoint_Builder identifier(UUID identifier)
+AnnotationPoint_Builder lastUpdated(Instant lastUpdated)
+AnnotationPoint_Builder notes(str notes)
+AnnotationPoint_Builder source(QualifiedPath source)
+AnnotationPoint_Builder startTime(Instant startTime)
}
class MetadataPoint {
+MetadataPoint(value, quality, timestamp, source)
+static Builder builder(MetadataPoint metadataPoint)
+static MetadataPoint empty(QualifiedPath source)
+QualityCode quality()
+MetadataPoint withSource(QualifiedPath source)
}
class MetadataPoint_Builder {
+MetadataPoint_Builder()
+MetadataPoint_Builder addValue(PropertyValue value)
+MetadataPoint build()
+MetadataPoint_Builder quality(QualityCode quality)
+MetadataPoint_Builder source(QualifiedPath source)
+MetadataPoint_Builder timestamp(Instant timestamp)
+MetadataPoint_Builder values(PropertySet values)
}
class StandardComplexPointType {
<<enum>>
+StandardComplexPointType ANNOTATION
+StandardComplexPointType GENERIC
+StandardComplexPointType METADATA
+Class getPointClass()
+Class getQueryOptionsClass()
+List~StandardComplexPointType~ values()
}
class Record {
}
class Enum {
}
SnapshotCapable <|-- DataPoint
TemporalPoint <|-- DataPoint
DataPoint <|-- AtomicPoint
Record <|-- Annotation
Record <|-- AnnotationPoint
Record <|-- MetadataPoint
TemporalPoint <|-- MetadataPoint
Enum <|-- StandardComplexPointType
AnnotationPoint o-- Annotation
AnnotationPoint ..> StandardComplexPointType
AnnotationPoint_Builder --> AnnotationPoint
MetadataPoint_Builder --> MetadataPoint
Class diagram for system.historian.types helper APIclassDiagram
class SystemHistorianTypes {
+AnnotationPoint annotationPoint(str source, Date startTime, Date endTime, str annotationType, str data, str identifier)
+AtomicPoint dataPoint(str source, object value, Date timestamp, int quality)
+MetadataPoint metadataPoint(str source, Dict properties, Date timestamp)
}
class AnnotationPoint {
}
class AtomicPoint {
}
class MetadataPoint {
}
SystemHistorianTypes ..> AnnotationPoint
SystemHistorianTypes ..> AtomicPoint
SystemHistorianTypes ..> MetadataPoint
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The new
system.historian.typeshelpers are currently effectively no-ops (annotationPointignores all arguments anddataPoint/metadataPointdon’t construct or return anything), which will lead to confusing runtime behavior; either wire these through to the underlying Java APIs or explicitly raiseNotImplementedErrorso misuse fails loudly. - In
AnnotationPoint.__init__andMetadataPoint.__init__, theprint(...)calls introduce unexpected side effects for what appear to be model/typing shims; consider removing these and, if needed, storing arguments on private attributes instead to keep the module side-effect free. - For the builder-style methods on
AnnotationPoint.BuilderandMetadataPoint.Builderin the runtime module, the bodies are emptypassimplementations; aligning them with the stubs by returningself(or raisingNotImplementedError) will prevent confusing attribute errors orNoneTypeissues at runtime.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `system.historian.types` helpers are currently effectively no-ops (`annotationPoint` ignores all arguments and `dataPoint`/`metadataPoint` don’t construct or return anything), which will lead to confusing runtime behavior; either wire these through to the underlying Java APIs or explicitly raise `NotImplementedError` so misuse fails loudly.
- In `AnnotationPoint.__init__` and `MetadataPoint.__init__`, the `print(...)` calls introduce unexpected side effects for what appear to be model/typing shims; consider removing these and, if needed, storing arguments on private attributes instead to keep the module side-effect free.
- For the builder-style methods on `AnnotationPoint.Builder` and `MetadataPoint.Builder` in the runtime module, the bodies are empty `pass` implementations; aligning them with the stubs by returning `self` (or raising `NotImplementedError`) will prevent confusing attribute errors or `NoneType` issues at runtime.
## Individual Comments
### Comment 1
<location path="src/com/inductiveautomation/historian/common/model/data/__init__.py" line_range="237" />
<code_context>
+ ):
+ # type: (...) -> None
+ super(MetadataPoint, self).__init__()
+ print(value, quality, timestamp, source)
+
+ @staticmethod
</code_context>
<issue_to_address>
**issue:** MetadataPoint constructor printing has the same runtime/logging concerns as in AnnotationPoint.
As with `AnnotationPoint`, printing all constructor arguments here introduces unexpected side effects and may unnecessarily expose metadata. Since this module is just a shim over the Java types, `__init__` should ideally do nothing (or the bare minimum) and avoid printing/logging entirely.
</issue_to_address>
### Comment 2
<location path="src/system/historian/types.py" line_range="50-51" />
<code_context>
+ An annotation point object that can be passed to
+ system.historian.storeAnnotations.
+ """
+ builder = AnnotationPoint.builder()
+ return builder.build()
+
+
</code_context>
<issue_to_address>
**issue (bug_risk):** annotationPoint ignores all provided arguments, which contradicts the API and docstring.
The function accepts `source`, `startTime`, `endTime`, `annotationType`, `data`, and `identifier`, but none are set on the builder, so the returned `AnnotationPoint` ignores caller input. Please either populate the builder with these values (converting to the appropriate Java types like `QualifiedPath`, `Instant`, and `UUID`) or raise `NotImplementedError` instead of returning an incomplete object.
</issue_to_address>
### Comment 3
<location path="src/system/historian/types.py" line_range="54-63" />
<code_context>
+ return builder.build()
+
+
+def dataPoint(
+ source, # type: Union[str, unicode]
+ value, # type: object
+ timestamp=None, # type: Optional[Date]
+ quality=None, # type: Optional[int]
+):
+ # type: (...) -> AtomicPoint
+ """Creates a data point that can be stored to a historian.
+
+ Data points represent individual values associated with a historical
+ path, timestamp, and quality.
+
+ Args:
+ source: The historical path where the data point will be stored.
+ value: The value to be stored in the historian.
+ timestamp: The timestamp when the data point was recorded. If
+ omitted, the current time is used. Optional.
+ quality: The quality code of the data point. If omitted, a
+ "good" quality is used. Optional.
+ """
+ pass
+
+
</code_context>
<issue_to_address>
**issue (bug_risk):** dataPoint is declared to return an AtomicPoint but currently does nothing.
The stub and docstring promise an `AtomicPoint`, but the function body is just `pass`, so it always returns `None`. This will cause runtime failures wherever an `AtomicPoint` is expected. Either raise `NotImplementedError` until it’s implemented, or construct and return the appropriate `AtomicPoint` (likely via the Java APIs) to match the declared return type.
</issue_to_address>
### Comment 4
<location path="src/system/historian/types.py" line_range="77-86" />
<code_context>
+ pass
+
+
+def metadataPoint(
+ source, # type: Union[str, unicode]
+ properties, # type: Dict[Union[str, unicode], Any]
+ timestamp, # type: Date
+):
+ # type: (...) -> MetadataPoint
+ """Creates a metadata point that can be stored to a historian.
+
+ Metadata points allow you to store additional properties associated
+ with a historical path at a specific point in time.
+
+ Args:
+ source: The historical path where the metadata point will be
+ stored.
+ properties: A dictionary of properties to be stored as
+ historical metadata.
+ timestamp: The timestamp when the metadata point was recorded.
+
+ Returns:
+ A metadata point object that can be passed to
+ system.historian.storeMetadata.
+ """
</code_context>
<issue_to_address>
**issue (bug_risk):** metadataPoint never constructs or returns a MetadataPoint despite the signature.
Because the body only contains a docstring, this function currently returns `None` despite the `MetadataPoint` return annotation. Please either implement the construction of a `MetadataPoint` or explicitly `raise NotImplementedError` so misuse is caught immediately rather than silently returning `None`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ): | ||
| # type: (...) -> None | ||
| super(MetadataPoint, self).__init__() | ||
| print(value, quality, timestamp, source) |
Contributor
There was a problem hiding this comment.
issue: MetadataPoint constructor printing has the same runtime/logging concerns as in AnnotationPoint.
As with AnnotationPoint, printing all constructor arguments here introduces unexpected side effects and may unnecessarily expose metadata. Since this module is just a shim over the Java types, __init__ should ideally do nothing (or the bare minimum) and avoid printing/logging entirely.
Comment on lines
+50
to
+51
| builder = AnnotationPoint.builder() | ||
| return builder.build() |
Contributor
There was a problem hiding this comment.
issue (bug_risk): annotationPoint ignores all provided arguments, which contradicts the API and docstring.
The function accepts source, startTime, endTime, annotationType, data, and identifier, but none are set on the builder, so the returned AnnotationPoint ignores caller input. Please either populate the builder with these values (converting to the appropriate Java types like QualifiedPath, Instant, and UUID) or raise NotImplementedError instead of returning an incomplete object.
Comment on lines
+54
to
+63
| def dataPoint( | ||
| source, # type: Union[str, unicode] | ||
| value, # type: object | ||
| timestamp=None, # type: Optional[Date] | ||
| quality=None, # type: Optional[int] | ||
| ): | ||
| # type: (...) -> AtomicPoint | ||
| """Creates a data point that can be stored to a historian. | ||
|
|
||
| Data points represent individual values associated with a historical |
Contributor
There was a problem hiding this comment.
issue (bug_risk): dataPoint is declared to return an AtomicPoint but currently does nothing.
The stub and docstring promise an AtomicPoint, but the function body is just pass, so it always returns None. This will cause runtime failures wherever an AtomicPoint is expected. Either raise NotImplementedError until it’s implemented, or construct and return the appropriate AtomicPoint (likely via the Java APIs) to match the declared return type.
Comment on lines
+77
to
+86
| def metadataPoint( | ||
| source, # type: Union[str, unicode] | ||
| properties, # type: Dict[Union[str, unicode], Any] | ||
| timestamp, # type: Date | ||
| ): | ||
| # type: (...) -> MetadataPoint | ||
| """Creates a metadata point that can be stored to a historian. | ||
|
|
||
| Metadata points allow you to store additional properties associated | ||
| with a historical path at a specific point in time. |
Contributor
There was a problem hiding this comment.
issue (bug_risk): metadataPoint never constructs or returns a MetadataPoint despite the signature.
Because the body only contains a docstring, this function currently returns None despite the MetadataPoint return annotation. Please either implement the construction of a MetadataPoint or explicitly raise NotImplementedError so misuse is caught immediately rather than silently returning None.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce historian point type helpers and supporting model types while updating Java API dependencies.
New Features:
Enhancements:
Build: