New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for Immutable types #85
base: master
Are you sure you want to change the base?
Support for Immutable types #85
Conversation
CreateInstance will allow for parameterless constructor or a constructor with parameters to be used
Immutable types that are filled in from the constructor are now also possible to map from CSV data Syntax similar to existing MapProperty options
Add also support for MapUsing style syntax for immutable types. Multiple MapUsing can be configured and the first that succeeds will be used, if no MapUsing is configured or none succeeeds then it will default to the Activator implementation.
@@ -0,0 +1,350 @@ | |||
## Ignore Visual Studio temporary files, build results, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to add this content in the subfolder TinyCsvParser/.gitignore
. We can also decide to move the .gitignore
to top-level, I just did this for my aesthetic preferences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not even notice there was a .gitignore file in there so I added this template I have
I was getting a lot of extra files in the staging window but I am not sure why VS did not pick up on the existing file (probably some missing configuration on my side)
I am OK with removing it from the root level
List<object> args = new List<object>(); | ||
// TODO build constructor arguments | ||
|
||
TEntity entity = (TEntity)Activator.CreateInstance(typeof(TEntity), args.ToArray()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be better to pass the object creator in the constructor. Right now the constructor only contains the ITypeConverterProvider typeConverterProvider
, but we could also add a Func<TEntity>
lambda to create the target object.
But honestly, looking at it the CsvMapping
looks way too convoluted, we can simplify it a lot I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it would be nice to be able to pass the object creation logic to the CsvMapping constructor
Do you propose something like a Func<TokenizedRow, TEntity> lambda like the one in the new MapUsing method and replace the list with a single variable filled in from the constructor? Or how do you see it?
protected CsvRowConstructor<TEntity> MapUsing(Func<TokenizedRow, TEntity> action)
{
var rowConstructor = new CsvRowConstructor<TEntity>(action);
csvUsingConstructorMappings.Add(rowConstructor);
return rowConstructor;
}
@JoaoSeverino I am sorry it takes so long to review your PR. You deserve better for your work, I will try to find the time, but at the moment I cannot give a real timeline. 😞 |
@bytefish No worries, I am glad to assist and there is no need for timelines |
Resolves #84
Implements MapUsing and MapConstructorParameter to account for different syntax preferences