(GH-538) Define newtypes for tags field#1382
(GH-538) Define newtypes for tags field#1382michaeltlombardi wants to merge 3 commits intoPowerShell:mainfrom
tags field#1382Conversation
6840532 to
c3026f6
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces two newtypes (Tag and TagList) to replace the previous Option<Vec<String>> representation for tags in resource and extension manifests. The change implements the "parse, don't validate" pattern by moving tag validation into the constructor and enables case-insensitive tag comparisons while providing canonical JSON schemas for these types.
Changes:
- Defines
Tagnewtype wrappingStringwith regex validation (^\w+$) and case-insensitive comparison/hashing - Defines
TagListnewtype wrappingHashSet<Tag>to ensure uniqueness and provide reusable schema - Updates
ResourceManifestandExtensionManifestto useTagListinstead ofOption<Vec<String>> - Adds comprehensive integration tests following the repository's testing patterns
- Updates tag filtering logic in
list_resourcesto leverage case-insensitive comparison
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/dsc-lib/src/types/tag.rs | Defines Tag newtype with validation pattern ^\w+$, case-insensitive comparison/hashing, and extensive trait implementations |
| lib/dsc-lib/src/types/tag_list.rs | Defines TagList newtype wrapping HashSet<Tag> with ergonomic trait implementations |
| lib/dsc-lib/src/types/mod.rs | Exports new Tag and TagList types |
| lib/dsc-lib/src/extensions/extension_manifest.rs | Changes tags field from Option<Vec<String>> to TagList with #[serde(default)] |
| lib/dsc-lib/src/dscresources/resource_manifest.rs | Changes tags field from Option<Vec<String>> to TagList with #[serde(default)] |
| lib/dsc-lib/src/dscerror.rs | Adds InvalidTag error variant for tag validation failures |
| lib/dsc-lib/locales/en-us.toml | Adds localized error message keys for invalid tag errors |
| lib/dsc-lib/locales/schemas.definitions.yaml | Adds comprehensive documentation for tag and tags schema definitions |
| dsc/src/subcommand.rs | Updates tag filtering to use is_empty() and case-insensitive comparison via Tag type |
| lib/dsc-lib/tests/integration/types/tag.rs | Comprehensive integration tests for Tag covering methods, schema, serde, and traits |
| lib/dsc-lib/tests/integration/types/tag_list.rs | Comprehensive integration tests for TagList covering schema, serde, and traits |
| lib/dsc-lib/tests/integration/types/mod.rs | Registers new test modules for tag and tag_list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c3026f6 to
329dde3
Compare
329dde3 to
7f56f8c
Compare
7f56f8c to
7434453
Compare
7434453 to
32574d9
Compare
32574d9 to
8bd3234
Compare
8bd3234 to
2e78608
Compare
2e78608 to
8ec0868
Compare
8ec0868 to
44c3c8c
Compare
Prior to this change, the representation for both resource and extension tags was defined as `Option<Vec<String>>`. With this construction, we can't define a reusable schema for tag lists (because we don't own the `JsonSchema` trait or the `Option<T>` type). The existing implementation also doesn't adhere to the validation from the canonical JSON Schema for tags, which defines the regex pattern `^\w+$` for tags. This change addresses these limitations by defining two newtypes: - `Tag` as a wrapper around `String` that follows the "parse, don't validate" pattern to move pattern validation into the constructor. It also treats comparisons with tags case-insensitively. The defined JSON Schema for this type is marked as `inline` because it is always and only used to validate items in the `tags` field for various structs. - `TagList` as a transparent wrapper around `HashSet<Tag>`. It provides no additional functionality, it just enables us to define a JSON Schema for this reusable type. The representation here uses `HashSet` rather than `Vec` because the items in a tag list must be unique (JSON Schema keyword `uniqueItems` defined as `true`). We can still skip serializing when the `HashSet` is empty, like we can skip serializing when an `Option` is `None`.
Prior to this change, the resource manifest struct defined the `tags` field as `Option<Vec<String>>`. This change updates the field to use the `TagList` newtype, which wraps `HashSet<Tag>` to provide better semantics and JSON Schema for the type. This change ensures that references to the `tags` field are updated through the code base.
Prior to this change, the extension manifest struct defined the `tags` field as `Option<Vec<String>>`. This change updates the field to use the `TagList` newtype, which wraps `HashSet<Tag>` to provide better semantics and JSON Schema for the type. The `tags` field isn't directly accessed in any existing code.
44c3c8c to
87798d9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR Summary
This change addresses limitation in representing the
tagsfield by defining two newtypes:Tagas a wrapper aroundStringthat follows the "parse, don't validate" pattern to move pattern validation into the constructor. It also treats comparisons with tags case-insensitively. The defined JSON Schema for this type is marked asinlinebecause it is always and only used to validate items in thetagsfield for various structs.TagListas a transparent wrapper aroundHashSet<Tag>. It provides no additional functionality, it just enables us to define a JSON Schema for this reusable type. The representation here usesHashSetrather thanVecbecause the items in a tag list must be unique (JSON Schema keyworduniqueItemsdefined astrue).This change modifies the code for the
ResourceManifestandExtensionManifeststructs to useTagListinstead ofOption<Vec<String>>and updates other code as necessary to address the change in type.PR Context
Prior to this change, the representation for both resource and extension tags was defined as
Option<Vec<String>>. With this construction, we can't define a reusable schema for tag lists (because we don't own theJsonSchematrait or theOption<T>type).The existing implementation also doesn't adhere to the validation from the canonical JSON Schema for tags, which defines the regex pattern
^\w+$for tags.