Allow slashes in DataMarkerAttributes#7890
Conversation
- Modified DataMarkerAttributes::validate to allow '/' in attributes.
- Added a nested attribute example ("nested/part") to HelloWorldProvider.
- Added a doc-test for the nested attribute.
Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Relaxed assertion in FsDataProvider to allow forward slashes in attributes, with safety checks. - Updated baked and blob test data for HelloWorldV1. Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Updated DataMarkerAttributes::validate to disallow leading slashes and double slashes. - Added unit tests for these new invariants. Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Defaulted prev_was_slash to true to simplify leading slash check. Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Manishearth
left a comment
Manishearth
left a comment
There was a problem hiding this comment.
just a code review. not yet sure if we should support this but ... probably? I thought we didn't end up picking the nested attribute designs for DisplayNames, and I have a slight preference to only make this change when we actually need it. But maybe we need it now, and I'm not strongly opposed to doing this proactively.
We said that length and region could be nested in the attributes, but I had already implemented length on the key level and didn't go back to revisit it yet. |
| #[repr(transparent)] | ||
| pub struct DataMarkerAttributes { | ||
| // Validated to be non-empty ASCII alphanumeric + hyphen + underscore | ||
| // Validated to be non-empty ASCII alphanumeric + hyphen + underscore + forward slash. Disallows leading and double slashes. |
There was a problem hiding this comment.
it should also disallow trailing slashes
There was a problem hiding this comment.
I agree, otherwise this leads to multiple attributes with the same fs path.
Manishearth
left a comment
Manishearth
left a comment
There was a problem hiding this comment.
If we have a current use case, sure. LGTM with the trailing slashes fixed
- Moved empty string check to the top of `validate`. - Added comment to clarify trailing slash check. - Added empty string as a valid test case. Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>
3 participants
This is for multi-layered attributes, like "FR/long", which we may want for DisplayNames.
I made this with a heavily-prompted agent.
Changelog
icu_provider: allow slashes in DataMarkerAttributes