Added DelimitedFieldPaddingAttribute #373
Conversation
There was a problem hiding this comment.
Hi there
Thank you for your pullrequest and the tests that show how your feature works.
- Your attribute is named "Padding" but then the parameter is alignment. I find that confusing.
- The logic is deeply integrated into FieldBase and DelimitedField. That will complicate maintenance of the library.
-- most of the logic could be moved to a converter, which then could be configured, see https://www.filehelpers.net/example/Converters/CustomConverter/
--- however with customconverters you cannot do the padding logic and conversion of types, for example DateTime formatting and then padding to a length.
--- also parameters to the converter would not be type safe, they are just strings or object param arrays.
Matthias
| var engine = new FileHelperEngine<AlignLeftSample>(); | ||
| string output = engine.WriteString(records).TrimEnd(); | ||
|
|
||
| string expected = "AA ,100 ,2020-01-01**"; |
There was a problem hiding this comment.
I think the expectation should be the other way round.
The .Net string behaves as follows
var text = "AA".PadLeft(6);
// text is now " AA"
| var engine = new FileHelperEngine<AlignCenterSample>(); | ||
| string output = engine.WriteString(records).TrimEnd(); | ||
|
|
||
| string expected = "++AA++,+100+,+2020-01-01+"; |
There was a problem hiding this comment.
Given the value would be "AAA" and the field length would be 4:
" AAA" or "AAA " ?
| var engine = new FileHelperEngine<AlignRightSample>(); | ||
| string output = engine.WriteString(records).TrimEnd(); | ||
|
|
||
| string expected = " AA,**100,++2020-01-01"; |
There was a problem hiding this comment.
Same as left above: I think it should be the other way round.
Is there a way to avoid integrating the feature into the FieldBase and DelimitedField attributes and still allow for the converter to a) only apply to delimited fields and b) be compatible with also applying a converter such as for date conversions?
I avoided using a custom converted by design since the IMO the feature should allow a converter to run on the field and then apply padding. Also if a custom converter is used there could be confusion when applying padding to a fixed width field. IIRC fixed width field padding is supported by a separate attribute which is not compatible with delimited fields.
That is why I opted to use a new attribute rather than a custom converter.
To be clear I will be happy to re-write the code. I'm looking for feedback on how to approach it to best fit with the desired feature and style. Thanks for the feedback. |
|
Hi Jeremy Still, I find a padding feature on a delimited feature confusing. And if we really need it, would it not be more intuitive to allow fixed fields on delimited records? However I have simplified the extension with converters #380. It should significantly simplify the creation and the type safeness of custom converters. It eliminates the indirection of Enum.Custom to type and the attribute. Converters inherit from Attribute directly. This allows to create the converter directly on the field and hereby pass parameters to it in a type safe way. Reuse of the contained built-in converters could further be simplified by making their methods virtual. A converting and padding custom-converter could be written by inheriting the converter (transformation) and calling the overriden method. Another way to reuse the built-in converters without making its methods virtual would be a padding decorator. A converter would hold a reference to an other one, like double converter. After getting the double value, the padding decorator would pad the value. |
Resolves #149 by added a DelimitedFieldPaddingAttribute