MiniExcelDataReader refactoring#929
Conversation
Integrating the implementation of MiniExcelDataReader to fully support nearly all IDataReader methods and turn it into a fully functional data reader. The new implementation also supports asynchronous reads so there's no need for a separate MiniExcelAsynDataReader, which has thus been removed. The MiniExcelDataReaderBase abstract class has also been removed, as it is no longer needed given its purpose was to hide all the non implemented IDataReader methods from the main class. The OpenXmlImporter and CsvImporter classes have been updated to reflect these changes.
I'm pretty confident this write adapter has never been of much usefulness, but now that MiniExcelDataReader has become a fully implemented IDataReader I think there is no reason to keep it around.
…a reader There is no usefulness in forcing each field to be retrieved asynchronously since each row is fully loaded in memory anyways. Removing the superfluous IAsyncEnumerables cuts down a lot of overhead.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the data reading and writing mechanisms within MiniExcel. The primary goal was to consolidate synchronous and asynchronous data reader functionalities into a single, robust Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors and unifies synchronous and asynchronous data readers into a single MiniExcelDataReader class, improving IDataReader compliance and overall architecture, and enabling direct DataTable loading. A security audit found no vulnerabilities in the analyzed files. However, the review identified potential bugs in the new MiniExcelDataReader implementation and a possible regression in CsvImporter functionality.
- Reintating the CsvConfiguration parameter removed by mistake from the GeDataReader methods in the CsvImporter - Removing superfluous preprocessor instruction in IMiniExcelDataReader interface - Adding checks for empty data source and for correct buffering of char array in the MiniExcelDataReader class - Fixing test 408
|
@shps951023 @izanhzh please let me know what you think about these changes. |
|
👍👍👍 @michelebastione good day, It looks nice 🙌 |
|
@shps951023 Cool! Don't worry about it. Though I do have to mention that in #918 I managed to decouple the core logic from the OpenXml and Csv, which will make it a lot easier if we decide to add support for other formats like Ods in the future. This means that we will need to create a new nuget package for the next prerelease, if you don't have anything against it: MiniExcel.OpenXml. |
|
@michelebastione Sounds good! Please go ahead 🙌 |
3 participants
I've integrated the implementation of
MiniExcelDataReaderto fully support nearly allIDataReadermethods and turn it into a fully functional data reader. The new implementation also supports asynchronous reads so there's no need for a separateMiniExcelAsynDataReader, which has thus been removed. TheMiniExcelDataReaderBaseabstract class has also been removed, as its purpose was to hide all the non implementedIDataReadermethods from the mainMiniExcelDataReaderclass.The
OpenXmlImporterandCsvImporterclasses have been updated to reflect these changes.This PR fixes #408 as with this change it is now possible to load a
DataTablefrom aMiniExcelDataReader.Besides this main change, I've also made two other minor refactorings: the class
MiniExcelDataReaderWriteAdapterhas been removed and the logic for retrieving values from aIDataReaderin order to export them has been simplified.