Conversation
WalkthroughThis update introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Test Runner
participant ExtensionSerializerTests as ExtensionSerializerTests
participant ResourceStream as Embedded Resource Stream
participant Serializer as DataPackageSerializer
participant DataPackage as DataPackage
TestRunner->>ExtensionSerializerTests: Run Deserialize_Package_Success/Deserialize_Metrics_Success
ExtensionSerializerTests->>ResourceStream: Retrieve embedded JSON/YAML resource
ExtensionSerializerTests->>Serializer: Instantiate serializer (JSON/YAML)
ExtensionSerializerTests->>Serializer: Deserialize resource stream
Serializer->>DataPackage: Produce DataPackage object
ExtensionSerializerTests->>DataPackage: Assert package and metrics properties
Possibly related PRs
Suggested labels
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #175 +/- ##
==========================================
+ Coverage 76.88% 76.93% +0.04%
==========================================
Files 105 106 +1
Lines 1787 1795 +8
Branches 283 283
==========================================
+ Hits 1374 1381 +7
- Misses 318 319 +1
Partials 95 95 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
src/Packata.Core/Metric.cs (1)
33-38: Consider adding validation for aggregation functions.The
Aggregationproperty could benefit from validation constraints or enumeration of allowed values (SUM, MAX, COUNT, etc.) to ensure only supported aggregation functions are used.You could implement this by using an enum instead of a string:
- public string? Aggregation { get; set; } + public AggregationType? Aggregation { get; set; }And define the enum:
public enum AggregationType { SUM, MAX, MIN, COUNT, AVG // Add other supported aggregation types }src/Packata.Core/Schema.cs (1)
52-52: Consider initializing the Metrics property with an empty list.For consistency with the
Fieldsproperty (line 13), consider initializing theMetricsproperty with an empty list instead of leaving it as null by default. This would provide a more consistent API experience.- public List<Metric>? Metrics { get; set; } + public List<Metric> Metrics { get; set; } = [];src/Packata.Core.Testing/Serialization/Yaml/Resources/extension.yaml (2)
5-5: Creation date is set in the future.The creation date is set to "2025-05-04T12:45:21Z", which is in the future. While this may be intentional for testing purposes, it's worth noting that this could cause issues if temporal validation is implemented.
32-43: Good implementation of metrics array.The metrics array is well-structured with appropriate properties for name, type, title, description, aggregation, and expression. This aligns well with the feature goal of defining basic metrics.
However, I notice that the second metric
max_temperaturelacks a description property while the first one has it. Consider adding a description for consistency.- name: max_temperature type: numeric title: Maximum temperature + description: Maximum recorded temperature of the sensor aggregation: max expression: temperaturesrc/Packata.Core.Testing/Serialization/Json/Resources/extension.json (1)
48-64: Well-structured metrics definition.The metrics array is properly defined with appropriate properties, matching the structure in the YAML file. Good job on keeping the definitions consistent between formats.
Similar to the YAML file, the second metric
max_temperatureis missing a description property while the first one has it. Consider adding a description for consistency.{ "name": "max_temperature", "type": "numeric", "title": "Maximum temperature", + "description": "Maximum recorded temperature of the sensor", "aggregation": "max", "expression": "temperature" }src/Packata.Core.Testing/Serialization/ExtensionSerializerTests.cs (1)
20-28: Resource stream retrieval could be more robust.The method to retrieve resource streams is well-structured, but could be improved for maintainability.
private static Stream GetDataPackageProperties(string format) { - var uformat = format.ToUpper()[0] + format.Substring(1); + var uformat = char.ToUpper(format[0]) + format[1..]; var assembly = Assembly.GetExecutingAssembly(); var resourceName = $"{assembly.GetName().Name}.Serialization.{uformat}.Resources.extension.{format}"; - var stream = assembly.GetManifestResourceStream(resourceName) - ?? throw new FileNotFoundException($"The embedded file {resourceName} doesn't exist."); + var stream = assembly.GetManifestResourceStream(resourceName); + if (stream == null) + { + // For debugging purposes, list available resources + var resources = assembly.GetManifestResourceNames(); + var availableResources = string.Join(", ", resources); + throw new FileNotFoundException($"The embedded file {resourceName} doesn't exist. Available resources: {availableResources}"); + } return stream; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
package-delta-schemas.ps1(2 hunks)src/Packata.Core.Testing/Packata.Core.Testing.csproj(1 hunks)src/Packata.Core.Testing/Serialization/ExtensionSerializerTests.cs(1 hunks)src/Packata.Core.Testing/Serialization/Json/Resources/extension.json(1 hunks)src/Packata.Core.Testing/Serialization/Yaml/Resources/extension.yaml(1 hunks)src/Packata.Core/Field.cs(2 hunks)src/Packata.Core/Metric.cs(1 hunks)src/Packata.Core/Schema.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Packata.Core/Schema.cs (1)
src/Packata.Core/Metric.cs (1)
Metric(8-44)
🔇 Additional comments (8)
src/Packata.Core/Metric.cs (1)
1-44: NewMetricclass implementation looks good.The
Metricclass is well-structured with properly documented properties that capture all necessary aspects of metrics: name, type, title, description, aggregation function, expression, and dimensions.src/Packata.Core/Field.cs (3)
16-16: Documentation fix for Type property.Removed extraneous quotes from the documentation comment.
26-26: Documentation fix for Title property.Removed extraneous quotes from the documentation comment.
31-31: Documentation fix for Description property.Removed extraneous quotes from the documentation comment.
src/Packata.Core/Schema.cs (1)
52-52: Added Metrics property to Schema class.The new nullable
Metricsproperty allows schemas to include metric definitions, complementing the newMetricclass.src/Packata.Core.Testing/Packata.Core.Testing.csproj (2)
2-6: Explicit exclusion of PathHandling directory.You're now explicitly excluding the PathHandling directory from compilation, embedded resources, and None item groups, which helps maintain a clean project structure.
9-10: Added new test resources for metrics functionality.New embedded resources
extension.jsonandextension.yamlhave been added to support testing the new metrics functionality.Also applies to: 14-16
package-delta-schemas.ps1 (1)
3-8: Refactored URL handling with a centralized base URL.Good refactoring to extract the common base URL into a variable, which makes the code more maintainable.
| try { | ||
| Write-Host "Generating schema for $($schema.Class)..." | ||
| schemathief delta -a $assemblyPath -c $schema.Class -b $schema.Url -x "paths|profile" -o $schema.Output | ||
| schemathief delta -a $assemblyPath -c $schema.Class -b ($schema.Url + $schema.Value) -x "paths|profile" -o ($dir + $schema.Value) |
There was a problem hiding this comment.
Bug in schema generation command.
The script tries to use $schema.Url in the command, but there is no 'Url' property defined in the schema entries.
- schemathief delta -a $assemblyPath -c $schema.Class -b ($schema.Url + $schema.Value) -x "paths|profile" -o ($dir + $schema.Value)
+ schemathief delta -a $assemblyPath -c $schema.Class -b ($baseUrl + $schema.Value) -x "paths|profile" -o ($dir + $schema.Value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| schemathief delta -a $assemblyPath -c $schema.Class -b ($schema.Url + $schema.Value) -x "paths|profile" -o ($dir + $schema.Value) | |
| schemathief delta -a $assemblyPath -c $schema.Class -b ($baseUrl + $schema.Value) -x "paths|profile" -o ($dir + $schema.Value) |
| @@ -0,0 +1,44 @@ | |||
| "$schema": https://packata.dev/profiles/2.0/datapackage.json | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Schema version inconsistency detected.
The datapackage schema reference uses version 2.0 (https://packata.dev/profiles/2.0/datapackage.json), while the dataresource (line 13) and tableschema (line 44) both use version 1.0. This inconsistency should be resolved to maintain version compatibility.
🏁 Script executed:
#!/bin/bash
# Check for schema version inconsistencies in yaml files
echo "Checking schema versions in YAML files..."
find src -name "*.yaml" -exec grep -l "\$schema" {} \; | xargs cat | grep -o "profiles/[0-9]\.[0-9]"Length of output: 340
🏁 Script executed:
#!/bin/bash
grep -R "profiles/1.0" -n srcLength of output: 1149
Align schema version references in YAML extension
There’s a mismatch in extension.yaml between the datapackage schema (2.0) and the resource/tableschema schemas (1.0). All schema URLs should use the same version.
Files to update:
src/Packata.Core.Testing/Serialization/Yaml/Resources/extension.yaml- Line 1: currently
"https://packata.dev/profiles/2.0/datapackage.json" - Line 13:
"https://packata.dev/profiles/1.0/dataresource.json" - Line 44:
"https://packata.dev/profiles/1.0/tableschema.json"
- Line 1: currently
- For reference, JSON fixtures and code defaults use 1.0:
src/Packata.Core.Testing/Serialization/Json/Resources/extension.json(all schemas 1.0)Packata.Core/DataPackage.cs,Resource.Props.cs,TableDialect.cs(default profiles 1.0)
Suggested fixes (pick one):
- Downgrade the datapackage URL in
extension.yamlto 1.0 to match the others. - Or, if you’re ready to adopt 2.0, bump the dataresource and tableschema URLs (and JSON fixtures/code defaults) to 2.0.
Please update the URLs for consistent schema versions.
| @@ -0,0 +1,69 @@ | |||
| { | |||
| "$schema": "https://packata.dev/profiles/1.0/datapackage.json", | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Schema version inconsistency with YAML counterpart.
The JSON file uses version 1.0 for the datapackage schema, while the YAML counterpart uses version 2.0. This inconsistency should be resolved to maintain consistency between the two formats.
🏁 Script executed:
#!/bin/bash
# Compare schema versions between JSON and YAML files
echo "JSON schema version:"
grep -o "profiles/[0-9]\.[0-9]/datapackage.json" src/Packata.Core.Testing/Serialization/Json/Resources/extension.json
echo "YAML schema version:"
grep -o "profiles/[0-9]\.[0-9]/datapackage.json" src/Packata.Core.Testing/Serialization/Yaml/Resources/extension.yamlLength of output: 401
Align JSON schema version with YAML (2.0)
The JSON datapackage schema in extension.json is still on version 1.0, while the YAML counterpart uses 2.0. To keep both formats in sync, please update the JSON file:
- File: src/Packata.Core.Testing/Serialization/Json/Resources/extension.json
- Change line 2:
- Change line 2:
- "$schema": "https://packata.dev/profiles/1.0/datapackage.json",
- "$schema": "https://packata.dev/profiles/2.0/datapackage.json",
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "$schema": "https://packata.dev/profiles/1.0/datapackage.json", | |
| "$schema": "https://packata.dev/profiles/2.0/datapackage.json", |
| using var streamReader = new StreamReader(value.Stream); | ||
| var dataPackage = value.Serializer.Deserialize(streamReader, new LocalDirectoryDataPackageContainer(), new StorageProvider()); | ||
| Assert.That(dataPackage.Resources[0].Schema?.Metrics, Is.Not.Null); | ||
| var metrics = dataPackage.Resources[0].Schema?.Metrics; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check before accessing metrics collection.
While the code uses the null-conditional operator when retrieving the metrics, it doesn't ensure metrics is non-null before using it in assertions.
- var metrics = dataPackage.Resources[0].Schema?.Metrics;
+ var metrics = dataPackage.Resources[0].Schema?.Metrics;
+ Assert.That(metrics, Is.Not.Null, "The metrics collection should not be null");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var metrics = dataPackage.Resources[0].Schema?.Metrics; | |
| var metrics = dataPackage.Resources[0].Schema?.Metrics; | |
| Assert.That(metrics, Is.Not.Null, "The metrics collection should not be null"); |
| [TestCaseSource(nameof(GetData))] | ||
| public void Deserialize_Metrics_Success((Stream Stream, IDataPackageSerializer Serializer) value) | ||
| { | ||
| using var streamReader = new StreamReader(value.Stream); | ||
| var dataPackage = value.Serializer.Deserialize(streamReader, new LocalDirectoryDataPackageContainer(), new StorageProvider()); | ||
| Assert.That(dataPackage.Resources[0].Schema?.Metrics, Is.Not.Null); | ||
| var metrics = dataPackage.Resources[0].Schema?.Metrics; | ||
| Assert.Multiple(() => | ||
| { | ||
| Assert.That(metrics, Has.Count.EqualTo(2)); | ||
| Assert.That(metrics?[0].Name, Is.EqualTo("average_temperature")); | ||
| Assert.That(metrics?[0].Type, Is.EqualTo("numeric")); | ||
| Assert.That(metrics?[0].Title, Is.EqualTo("Average temperature")); | ||
| Assert.That(metrics?[0].Description, Is.EqualTo("Average temperature of the sensor")); | ||
| Assert.That(metrics?[0].Aggregation, Is.EqualTo("avg")); | ||
| Assert.That(metrics?[0].Expression, Is.EqualTo("temperature")); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance metric testing coverage.
The test verifies the first metric's properties thoroughly but doesn't check all properties of the second metric.
[TestCaseSource(nameof(GetData))]
public void Deserialize_Metrics_Success((Stream Stream, IDataPackageSerializer Serializer) value)
{
using var streamReader = new StreamReader(value.Stream);
var dataPackage = value.Serializer.Deserialize(streamReader, new LocalDirectoryDataPackageContainer(), new StorageProvider());
Assert.That(dataPackage.Resources[0].Schema?.Metrics, Is.Not.Null);
var metrics = dataPackage.Resources[0].Schema?.Metrics;
Assert.Multiple(() =>
{
Assert.That(metrics, Has.Count.EqualTo(2));
// First metric checks
Assert.That(metrics?[0].Name, Is.EqualTo("average_temperature"));
Assert.That(metrics?[0].Type, Is.EqualTo("numeric"));
Assert.That(metrics?[0].Title, Is.EqualTo("Average temperature"));
Assert.That(metrics?[0].Description, Is.EqualTo("Average temperature of the sensor"));
Assert.That(metrics?[0].Aggregation, Is.EqualTo("avg"));
Assert.That(metrics?[0].Expression, Is.EqualTo("temperature"));
+
+ // Second metric checks
+ Assert.That(metrics?[1].Name, Is.EqualTo("max_temperature"));
+ Assert.That(metrics?[1].Type, Is.EqualTo("numeric"));
+ Assert.That(metrics?[1].Title, Is.EqualTo("Maximum temperature"));
+ Assert.That(metrics?[1].Aggregation, Is.EqualTo("max"));
+ Assert.That(metrics?[1].Expression, Is.EqualTo("temperature"));
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [TestCaseSource(nameof(GetData))] | |
| public void Deserialize_Metrics_Success((Stream Stream, IDataPackageSerializer Serializer) value) | |
| { | |
| using var streamReader = new StreamReader(value.Stream); | |
| var dataPackage = value.Serializer.Deserialize(streamReader, new LocalDirectoryDataPackageContainer(), new StorageProvider()); | |
| Assert.That(dataPackage.Resources[0].Schema?.Metrics, Is.Not.Null); | |
| var metrics = dataPackage.Resources[0].Schema?.Metrics; | |
| Assert.Multiple(() => | |
| { | |
| Assert.That(metrics, Has.Count.EqualTo(2)); | |
| Assert.That(metrics?[0].Name, Is.EqualTo("average_temperature")); | |
| Assert.That(metrics?[0].Type, Is.EqualTo("numeric")); | |
| Assert.That(metrics?[0].Title, Is.EqualTo("Average temperature")); | |
| Assert.That(metrics?[0].Description, Is.EqualTo("Average temperature of the sensor")); | |
| Assert.That(metrics?[0].Aggregation, Is.EqualTo("avg")); | |
| Assert.That(metrics?[0].Expression, Is.EqualTo("temperature")); | |
| }); | |
| } | |
| [TestCaseSource(nameof(GetData))] | |
| public void Deserialize_Metrics_Success((Stream Stream, IDataPackageSerializer Serializer) value) | |
| { | |
| using var streamReader = new StreamReader(value.Stream); | |
| var dataPackage = value.Serializer.Deserialize(streamReader, new LocalDirectoryDataPackageContainer(), new StorageProvider()); | |
| Assert.That(dataPackage.Resources[0].Schema?.Metrics, Is.Not.Null); | |
| var metrics = dataPackage.Resources[0].Schema?.Metrics; | |
| Assert.Multiple(() => | |
| { | |
| Assert.That(metrics, Has.Count.EqualTo(2)); | |
| // First metric checks | |
| Assert.That(metrics?[0].Name, Is.EqualTo("average_temperature")); | |
| Assert.That(metrics?[0].Type, Is.EqualTo("numeric")); | |
| Assert.That(metrics?[0].Title, Is.EqualTo("Average temperature")); | |
| Assert.That(metrics?[0].Description, Is.EqualTo("Average temperature of the sensor")); | |
| Assert.That(metrics?[0].Aggregation, Is.EqualTo("avg")); | |
| Assert.That(metrics?[0].Expression, Is.EqualTo("temperature")); | |
| // Second metric checks | |
| Assert.That(metrics?[1].Name, Is.EqualTo("max_temperature")); | |
| Assert.That(metrics?[1].Type, Is.EqualTo("numeric")); | |
| Assert.That(metrics?[1].Title, Is.EqualTo("Maximum temperature")); | |
| Assert.That(metrics?[1].Aggregation, Is.EqualTo("max")); | |
| Assert.That(metrics?[1].Expression, Is.EqualTo("temperature")); | |
| }); | |
| } |
2 participants
Summary by CodeRabbit
Metricclass and aMetricsproperty on schemas.