Closed biegehydra closed 1 year ago
@mckaragoz I know the contributor, he is working for my company. Sorry I have not had time to review. He is smart college guy who I am training. He is new to submitting PR's to open source. Give your feedback as you see fit. I personally would suggest he break each new feature into a PR versus them all in one. I also would not want to have other dependencies in this style of repository and would want an alternative or pluggable solution. That is why I did not use FuzzySharp in my PR but I use it in my production app. I made a note of it in the code of what could be done.
Thanks for all your hard work on the repo!
I can remove the AutoMatching feature from FuzzySharp, I agree, it doesn't belong in this reposotiory. However, all other feature I added depend on mapping the CSVs to dynamic
objects then casting to IDictionary<string,object>
using CsvHelper, so that dependency would not be optional for this merge request. I would still argue that the benefit it provides--removing unmapped data and adding default values--is worth the added dependency. Exporting a CSV without removing the unmapped data provides limited real life uses. But, it's up to you guys whether you want to merge it, just thought I'd share and give you guys the option.
I removed the FuzzySharp dependency and rebased
Changes
Video showing change