obsolete several child elements removed in Office 2015 #1905
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR obsoletes several child elements and updates the code generator to add attributes such as ObsoleteAttribute to complex type classes, child elements, and attributes.
- Introduces a new dictionary (_attributeData) to map fully qualified names to attribute annotations.
- Extends signature of WriteType, WriteElement, and WriteAttributeProperty methods to conditionally emit additional attribute strings.
Comments suppressed due to low confidence (2)
gen/DocumentFormat.OpenXml.Generator.Models/Generators/Elements/DataModelWriterExtensions.cs:196
- [nitpick] The variable name 'ctAttributeData' is ambiguous; consider renaming it to something like 'classAttributeAnnotations' to more clearly indicate its purpose.
if (_attributeData.TryGetValue(element.Name.ToString(), out Dictionary<string, List<string>> ctAttributeData))
gen/DocumentFormat.OpenXml.Generator.Models/Generators/Elements/DataModelWriterExtensions.cs:244
- [nitpick] The variable name 'attrAttributeData' could be renamed to something like 'attributeAnnotations' for clarity, making it easier to distinguish from the class-level attribute data.
if (_attributeData.TryGetValue(element.Name.ToString(), out Dictionary<string, List<string>> attrAttributeData)
| { | ||
| { | ||
| "c:CT_PictureOptions/c:pictureOptions", | ||
| new List<string>() |
There was a problem hiding this comment.
These are duplicated, so it may be nice to have a readonly static list that can be reused
| delimiter.AddDelimiter(); | ||
| writer.WriteAttributeProperty(services, attribute); | ||
|
|
||
| if (_attributeData.TryGetValue(element.Name.ToString(), out Dictionary<string, List<string>> attrAttributeData) |
There was a problem hiding this comment.
you could make the dictionary have the type of element.Name (I think it's a TypedQName) and it'll make it a bit more performant. The keys you're creating will also automatically be converted to it as well
There was a problem hiding this comment.
thanks, that was a good idea. for SchemaAttributes, they aren't actually TypedQName but I decided to try using the Type property of the attribute metadata to construct a string that can convert to a TypedQName for the purposes of keys in the dictionary. It was either that or string.Empty which would have issues in parsing. So even though the Type property of the attribute metadata is actually the c# type (class), it works. If you have an alternative suggestion I'll try that.
There was a problem hiding this comment.
I'd prefer not to have the C# type class - it would be nice to have that a completely separate thing.
| { | ||
| public static class AttributeStrings | ||
| { | ||
| public const string ObsoletePropertyWarn = "[Obsolete(\"Unused property, will be removed in a future version.\", false)]"; |
There was a problem hiding this comment.
Do we need all these? looks like only two are used
There was a problem hiding this comment.
Not right now, but I added them to make clear the distinction in the different types of obsolescence so we follow some pattern in naming.
There was a problem hiding this comment.
hmm seems like a lot of duplicated text. You can use $"{}" in consts to reduce that
There was a problem hiding this comment.
Not a blocker, but I prefer to compose them from smallerbits of text to ensure they're built up the same.
3 participants
this pr obsoletes the properties mentioned in #1769
it also adds code to the generator to add code attributes like ObsoleteAttribute to complex type classes, child elements and attributes.