Skip to content

Conversation

@a8t3r
Copy link

@a8t3r a8t3r commented Oct 4, 2018

Recently I was trying to implement a csv exporting with the reactive spring boot 2.
Something like that:

Flux<Bean> source = ...
Windmill
 // throws IllegalStateException because toIterable is not allowed inside "non-blocking" threads
  .export(source.toIterable())

I've noticed that all exporters contain an entire state (rows) before performing logic.
So it was the primary reason why I decided to rewrite exporters and large half of public api:

  • separation between logic and state
  • fluent api for import and export via builders

I'm open for your remarks and comments ;)

@coveralls
Copy link

coveralls commented Oct 4, 2018

Coverage Status

Coverage increased (+6.6%) to 93.576% when pulling 0c79ea0 on a8t3r:master into b68f1e6 on Coreoz:master.

@amanteaux
Copy link
Member

Thank you for your contribution.
I will have a proper look at your pull request in a few days.

From the very quick look of it:

  • The public API looks ok :)
  • I would like to remove any Java reflection in the library: it is not necessary and when a bug occurs it makes it difficult to debug.

@amanteaux
Copy link
Member

Before doing a pull request, I advise you to create an issue to describe the problem and the proposed solution. Then when the solution is approved, then you can start doing the code.
Moreover your pull request contains a number of different changes (new export feature with API changes, changes in how columns are represented in export, changes in how files are written, API changes in the import part etc.), in the future (so not for this one), please make an issue/pull request per change. It is really easier to read and understand when changes are focused to one feature/issue only.
I guess we need anyway a contributor guide.

I have read thoroughly your contribution, here are the points I want to discuss before merging your request:

Exporter
By reading the code, I think I understand the use case: you want to be able to stream data without putting everything in-memory.
So why not enforce that the destination stream is set before writting any row?
It would remove the need for any isHeaderInitialized or intermediate bytes array.

One part of your request is to enforce column name unicity. What is the motivation behind that? This change reduces the usability of the library, now it is not possible anymore to have columns that shared the same name.

What is the reason for the renaming of the writeTo method to writeInto?

Importer
I see the beauty of having a behaviour closer than the export functionality.
However, does it really improve the usability for the end user?

Tests
Please do not create tests for code you did not change.
If you think some parts of the application are not tested enough, please open an issue first to discuss it.

WindmillTest.should_export_as_csv_with_header_by_properties what is tested here that was not tested before?

WindmillTest.should_export_as_xlsx_no_header and WindmillTest.should_export_as_xls_no_header must be factorized.

Code style

  • Some method comments were deleted whereas it takes a very long time to write... It serves as user documentation and must be kept.
  • The library must have as little dependencies to other libraries as possible. That means that commons collections4 or lang3 should not be used in the main code.
  • In all the existing code, indentation is done with 1 tab, it should be respected (so not spaces, nor 2 tabs).
  • TODOS must not be added on existing code, if there is an issue in the library, please open a Github issue.
  • Existing imports order must not be changed.
  • Even in tests, deprecated code should not be used: it will need to be maintained later in the life of the project.

Documentation

This part should be done at the very end.
Currently there are multiple issues:

  • imports examples are mixed with exports examples,
  • some examples are overlapping with others,
  • some examples are here without any explanation,
  • in the import examples, Input streams are not closed.

@a8t3r a8t3r closed this Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants