Refactoring & Return nested Sheet Collection instead of flat Rows

[Harti]

My shot on fixing #17.

There are still a couple of things to discuss about this repo's strategy, and the resulting import() method signature will become convoluted as features get added. But it's still 0.2.1, so that should be fine and understandable.

After merging this, I would suggest to work on how the lib is instantiated. I'm thinking maybe pulling the $path variable into FastExcel.php itself so that the method changes to (new FastExcel('myfile.xlsx'))->doSomething. I can see how you wanted to maintain flexibility as to when an import reads from (an export method saves to) a path, but as mentioned in my comment there are some caveats to my current approach.

Furthermore interesting to delve into would be passing the respective callbacks into the FastExcel object using public void setSheetListener($callback) or similar methods and removing those from the import() method signature. But I didn't develop that yet as I wanted to hear what you think.

I'll be happy to hear back from you!

[rap2hpoutre]

Hi and thanks for your contribution!

There are still a couple of things to discuss about this repo's strategy, and the resulting import() method signature will become convoluted as features get added. But it's still 0.2.1, so that should be fine and understandable.

One of the main purpose of this package is to provide a super simple API for beginners. Returning the collection in import function seems to be easy. Just run import and you get a collection (and the lib don't care about sheets now). So I don't want to break this behavior.

When I opened issue #17, I thought about just moving these lines (from https://github.com/rap2hpoutre/fast-excel/blob/master/src/Importable.php#L56 to https://github.com/rap2hpoutre/fast-excel/blob/master/src/Importable.php#L75) to a sub function and don't change the API.

I'm still OK to discuss about this point, but I think a simple sub-function would do the job for now (maybe we will add multiple sheets management later).

Anyway, thanks for your PR and sorry for late answer! Feel free to ask me anything!

[rap2hpoutre]

Issue #17 was fixed via 8f49bfa. Thus, this PR is obsolete. Feel free to ask if you feel I should not close this PR.

[Harti]

The behavior would not have been broken through this PR (or, because I couldn't run the tests and can't say for sure, at least not if we tweaked it so); it would have been enhanced by a method to walk through the file's sheets if the developer desires that.

It turns out I misunderstood what you wanted to do with #17 after all. I thought you wanted to expose the ability to import the datasets sheet by sheet for the developer.

My use case in particular encompasses the import of Excel sheets ("user-readable backups" of their mobile app database; contains N sheets for the various tables that are structured like a relational database table with varying amounts and names of columns) - and I would later have to reverse this transformation, too.
Your use case seems to be either the display of (or the filling of a database with) Excel files split by Eloquent Model (so if you have N tables to fill, you'd have N Excel files as well). In my case, I have 1 Excel file, and N sheets.

I'm okay with you going the "developers are dumb, it needs to be absolutely simple and must not expose enhanced methods for the special ones" way.
This is not supposed to sound like a threat, however in that case I'll have to part ways with this library here and maybe think of creating a spin-off for people that require this as well. I'd rather not compete though, because the only thing I care about in the end of the day are my import and export scripts running on something modular and maintainable.

[rap2hpoutre]

Thank you for your answer and your thought! I really appreciate your feedback and sorry for the quick PR close.

I'm okay with you going the "developers are dumb, it needs to be absolutely simple and must not expose enhanced methods for the special ones" way.

Oh no, I don't think dev are dumb! I like (opinon mode ON) when things are simple to write for simple case. So I like the (new FastExcel)->import('a.xlsx') with nothing more. Not because people are dumb, but because I can read it, remember it and use it without knowing the library for most common cases (I guess). Still I think I misunderstood your PR: you did not add complexity, I missed something, I should have read it more carefully.

This is not supposed to sound like a threat, ...

haha that's OK!

My use case in particular encompasses the import of Excel sheets ("user-readable backups" of their mobile app database; contains N sheets for the various tables that are structured like a relational database table with varying amounts and names of columns) - and I would later have to reverse this transformation, too.

OK, I think it's a interesting case! I feel like I could imagine something simple for export:

// Create a collection of sheets
$sheets = new SheetCollection(User::all(), Project::all());

( new FastExcel($sheets) )->export('file.xlsx');

... but I'm not sure you will like it! And maybe it's objectively ugly. Feel free to criticize, what's your opinion? I created an issue here #29, but I'm not sure I want it like this anymore!

And maybe for import:

$sheets = (new FastExcel(SheetCollection::class))->import('file.xlsx');
// ... or ...
$sheets = (new FastExcel)->import('file.xlsx', SheetCollection::class);
// ... or ...
$sheets = (new FastExcel)->importAsSheetCollection('file.xlsx');

But not sure it's elegant nor readable...

Let me know if I misunderstood something, I would be happy to have your thoughts! And I'm glad to have you either as a competitor or a contributor!

[Harti]

Continuing your striving for elegance, I would prefer the syntax to become something like this:

$collection = FastExcel::import('a.xlsx');
// ... or ...
$sheetCollection = FastExcel::sheets('a.xlsx');
// ... or ...
$sheetCollection = FastExcel::import('a.xlsx'); // FastExcel detects whether the file contains multiple sheets, and, if so, gives you all of them.

with the (new FastExcel) part being handled by a Laravel Facade.

Another possibility would be to make a capsule class to make it similar to a query builder or collection object. For instance (pseudo-code):

public class FECollection {
   // these variables would be filled at runtime and could look something like this

   // if the file has multiple sheets, go recursive (because each sheet has headers)
   private $sheets = collect([ 'SheetA' => FECollection, 'SheetB' => FECollection, 'SheetC' => FECollection ]);
   private $headerRow = null;
   private $rows = null;

   // if the file has only one sheet (or this is a sheet), the rows/data are here
   private $sheets = null;
   private $headerRow = Collection;
   private $rows = Collection;


   // to keep it easy to use
   public function get($sheetName = null) : Collection
   {
		// current default behavior: return header row and the data of a single sheet
	   if(is_null($this->sheets))
	   {
		   return $this->headerRow ? $this->headerRow->concat($this->rows) : $this->rows;
	   }
	   
		// current default behavior: return header row and the data of the first sheet
		if(is_null($sheetName))
		{
			return $this->sheets->first()->get();
		}

		// return header row and the data of a select single sheet whose name was provided using the $sheetName parameter
		return $this->sheets->get($sheetName)->get();
   }

   public function header() : Collection
   {
		return $this->headerRow;
   }

   public function rows() : Collection
   {
		return $this->rows;
   }

   public function sheets() : Collection
   {
     return $this->sheets;
   }

 // ... more convenience functions
}

Then, you could make import(...) generate a FECollection object which would have more control about when/when not to include headers.

Access the data using:

// invoke current default behavior
FastExcel::import('a.xlsx')->get();  // === (new FastExcel)->import('a.xlsx');

// import header+rows from SheetC 
FastExcel::import('a.xlsx')->get('SheetC');

// import all sheets and do whatever you want
FastExcel::import('a.xlsx')->sheets();

Of course, backwards compatibility could still be kept by retaining the legacy invocation. This would simply be a new API.

Like you also suggested, it would be possible to build this FECollection object yourself:

new FECollection($rows);

new FECollection($rows, $headers);

new FECollection()
	->add($sheetName, $rows, $headers)
	->add($sheetName, $rows, $headers)
	->add($sheetName, $rows, $headers);

which you then could $feCollection->export('a.xlsx'); or FastExcel::export($feCollection)->to('a.xlsx');.

Let me know what you think, I'm sorry I didn't go detailed enough with my code but it's best to discuss it at a lower, non-functional level first to save time.

[rap2hpoutre]

Thank you! I will take some time tomorrow to answer! I re-open the pull request to make this discussion visible.

[rap2hpoutre]

Ok, thank you again for your contribution. So:

• I like your suggestion about a Façade (maybe just a static class à la Façade, not sure right now). (new FastExcel) is a bit weird (I thought it was OK, but not elegant though).
• I still like my suggestion about creating a SheetCollection : it can be used either as a result in import function, and as a param for export function. So for the user: use a collection if you want to work with one sheet, and a sheetcollection if you want to work with more. It seems simple and expressive.
• My imports suggestion for multiple sheets were not good, yours are better! Still, I think just using import that can return either a Collection or a SheetCollection depending on context is too much magic. Maybe later, but for now, I'm OK for import (return a collection) and importSheets (returns a sheet collection). Maybe later we will improve that.

---

So I suggest to split in independent (but ordered I guess) sub-tasks, I will open an issue for each:

1. Create a class SheetCollection that extends Collection with nothing more for now (so an empty class). It prepares the evolution of export/import system.
2. Improve "export" system, to accept either a collection and a sheet collection (a SheetCollection is just a collection of collections, but identified by its class). For now, it's in constructor, but it should be improved in a near future, because it's ugly and we will add a Façade.
3. Improve the "import" system by adding a importSheets function that returns N collections (one for each sheet) in a SheetCollection.
4. Create a Façade and preserve the backward compatibility. The only "new" thing to add to the Façade is about the constructor user in export. Now it needs a method called collect (or something else. So (new FastExcel($a))->export('a.xlsx') could be FastExcel::collect($a)->export('a.xlsx').

With these changes we could do something like:

// Import
// ======

// Simple usage
$users = FastExcel::import('users.xlsx');

// "Advanced" usage (with multiple sheets).
$sheets = FastExcel::importSheets('excel_with_some_sheets.xlsx');

// Export
// ======

// Simple usage
$filename = FastExcel::collect(User::all())->export('users.xlsx');

// "Advanced" usage (with multiple sheets).
$sheets = new SheetCollection([
    User::all(), 
    Project::all(), 
    SomethingElse::all()
]);
$filename = FastExcel::collect($sheets)->export('users.xlsx');

This is only the very first steps of an improvement based on your suggestion. Do you feel it could be a way to improve this library? Would you consider it as a average-good lib? Would you use it with that API ;)?

If you are roughly OK with this, I can create a millestone and a bunch of issue for this.

Thank you.

[Harti]

Sorry for the late reply on this. I really like the code block you posted. It would be quite lightweight both code- and API wise. And yes, I would certainly use it.

The Query Builder similarities I suggested would just make FastExcel behave more like an actual database driver, which is a known paradigm to Laravel developers and therefore easy to use. However, that can/could still be done at a later point of time - and certainly doesn't have to.

So, waaaaaay later, this could evolve to something like:

// this is just me being crazy!
$filtered = FastExcel::select(['id', 'name', 'salary'])->from('office.xlsx', 'WorkersSheet')->where('id', '>', 3)->get(); 

although frankly, one could simply use the Collection functions filter/reject etc. and do the same with that. This was just an idea of what could theoretically happen later on, but do not worry about it right now.

How would you "name" the sheets in your SheetCollection? Are you sure an "empty" wrapper class is sufficient?

(How do you make your code block get highlighted properly on GitHub?)

[rap2hpoutre]

Thank you for your answer!

(How do you make your code block get highlighted properly on GitHub?)

Try: ```php

How would you "name" the sheets in your SheetCollection? Are you sure an "empty" wrapper class is sufficient?

Hm, you are right, I did not thought about that. Maybe, first step is to automatically call them "sheet 1", "sheet 2" and so on. Then later we could create a richer class (optional) for these kind of need (a Sheet class).

The Query Builder similarities I suggested would just make FastExcel behave more like an actual database driver, which is a known paradigm to Laravel developers and therefore easy to use. However, that can/could still be done at a later point of time - and certainly doesn't have to.

That's a good idea. I will consider it later though.

although frankly, one could simply use the Collection functions filter/reject etc. and do the same with that. This was just an idea of what could theoretically happen later on, but do not worry about it right now.

:)

Anyway, thanks a lot for your suggestions! It really helps me in creating a better tool!

[Harti]

I don't know if something like this could work!
(I do require some sort of ability to adjust how my Export's sheets are called. It is unprofessional to hand an Excel file to users that contains "Sheet 1" instead of a semantically useful name, i.e. I would have to then open the Excel and rename the sheets using another library - but of course, we want FastExcel!)

public class SheetCollection extends Collection 
{
     private $sheetName;

    function __construct(Collection data, $name = null)
    {
        parent::__construct($data);

         if(is_null($name))
        {
             // auto-generate... sheet 1 / 2 / 3 ..,,
        }
        else 
        {
             $this->sheetName = $name;
        }
    }
}

What do you think?

[Harti]

@rap2hpoutre don't take my criticism as disrespectful, but in my opinion you can't just tell people "no" when they programmed enhancements, and at the same time put this "on hold", do nothing for four weeks, and expect people to program PRs for you (after you told them "no"). All that has happened so far was someone writing <?php class SheetCollection extends Collection {}. Wow, that's something. 👏 👏 👏 I couldn't have done that in 1 minute (except when I posted it in my previous comment). That that even counts as a contribution is ridiculous, but that's not my concern.

As far as I am concerned, I am not keen on programming something into the blue and hoping you end up liking it. But I do need progress to happen, because I have been waiting on this repo to fulfill my use case, and if it continues to not do so I am indeed programming my own solution (for my own). I have deadlines to keep.

I suggest you either start working on your own proposals, or you stop cockblocking other developers (by suggesting your ideas are better) and start helping to integrate their commits into the scheme you want it to be. But for little repos, doing nothing can actually be deadly.

[rap2hpoutre]

@Harti Oops, I'm really sorry.

I think you misunderstood my intention and my acts. I will take time to answer you sincerly as soon as possible. I'm really sad and sorry about your feeling. I thought everything was OK with our discussion and my attitude, but it was not, sorry. I believe we were building something together, I really apreciate the whole work we have done for this lib. I don't want to be the person you described. I hope I am not. I would like to change if so, help me if you want. Sorry for the waste of time too.

[Harti]

@rap2hpoutre Sorry about it sounding harsh like I'm making drama 😅 I'm not trying to pressure you. I agree that we had a good discussion about how things should and shouldn't go, but I was under the assumption that you're doing the next step (so that I can see which direction you're heading). If so, I have hereby informed you that my deadlines are closing in 😛 doesn't have to be today, not tomorrow, but sometime "soon", within the next month I need to know if I can use FastExcel for my projects.

Take your time, don't rush it, but do keep me updated on what you need from me 😃

[rap2hpoutre]

PS: I don't take your criticism as disrespectful, it's OK! I'm sad to be perceived that way, I'll try to be better.

[rap2hpoutre]

Take your time, don't rush it, but do keep me updated on what you need from me 😃

Anyway, I will answer you with a more detailed answer in this ticket. And I will do something for this lib.

Thank you for your kind words!

[rap2hpoutre]

you can't just tell people "no" when they programmed enhancements

Sorry about that feeling. I did not feel I just said "no". I posted two comments #24 (comment) then we discussed about it in a some other comments (after I closed it and wrote: "Feel free to ask if you feel I should not close this PR."). I first thought your PR was an attempt to fix an issue I opened without other goal than "let's participate to one project I like, here is my try" and I thought: "oh, maybe he misunderstood the original goal of the issue". But I was totally wrong, and I'm sorry for the waste of time (and my poor level in english, I know I misunderstand many things).

and at the same time put this "on hold"

I put it "on hold" 1 month after I closed (by mistake) the PR. I added this tag just one day after I actually re-opened the PR because I thought: "this discussion is interesting and it seems important to the OP, I must re-open the PR". Then, the day after I thought: "I must not merge this PR by mistake, it's a discussion, I will add a tag 'on hold'".

do nothing for four weeks

That's the main problem I guess. This repo was first (for me) a tool for one need at one job. Then I open-sourced it and shared it, because I wanted to create a tiny alternative to the defacto Laravel Excel. And I was happy it has some users, so I created some issues (for a kind of roadmap) and was happy it became a (really small) competitor to the original Laravel Excel. I did not read carefully some of your comments I guess and did not understand it was really professionally important for you. Then I continued my day to day job (currently, I work on a NodeJS project), without thinking too much to this project (I try to spend some time twice a week to answer issues and PR on open source projects I create). But I try to maintain all my projects and answer to every issues, PR, etc. Sometimes I fail.

and expect people to program PRs for you (after you told them "no")

I did not expected this! I was chatting with you about the evolution of the lib. I was super happy to have someone on board to talk with about the future of the lib (it's egoistic, but I thought you enjoyed too). I did not asked anyone for a PR (or maybe I did, but in this case, I don't remember and I apologize).

All that has happened so far was someone writing <?php class SheetCollection extends Collection {}. Wow, that's something. 👏 👏 👏 I couldn't have done that in 1 minute (except when I posted it in my previous comment). That that even counts as a contribution is ridiculous, but that's not my concern.

OK this part is important to me. Because I opened this issue to be inclusive with new comers. Some context: more than one year ago, I decided to learn the Rust programming language. It was super cool, but it seemed hard to me to understand some concepts. There were something called the Rust Libz Blitz where all projects opened simple issues that could be fixed by newcomers. It was a way for me to improve my level with this language, and I was super happy to just add one line, remove one obsolete comment, etc. because I had the feeling I participate and I improve my skills. The community was super kind with me. I tried to do the exact same thing with this issue. And I failed because you thought it's not kind to do such a thing: you already have done it. So, sorry about that.

I have to go back to my house, I will finish my answer tonight. And after that, I will try to improve this lib a bit.

[Harti]

Thank you for your detailed reply. Let me start with a big "sorry": I did not mean to upset you this much by stating what I did. I tried to keep my "complaint" short and therefore, some things sound worse than they were meant. I'm not a native English speaker either, so that's probably the main root of the misunderstandings 😅

I did not feel I just said "no"(...)

You didn't, and I remember our idea exchange! The gist of it, though, was a "not like this" and/or "not right now", so essentially a "no" from my point of view. I didn't take it personally, and the reasons you mentioned were good!

I'm sorry for the waste of time

You did not waste my time - it was fun to think of ideas on how to combine our two approaches into code. I would only think of it as a waste of my time if I started to program something like ABC, but you actually wanted XYZ. Because then, you won't be able to merge it.

This repo was first (for me) a tool for one need at one job. Then I open-sourced it and shared it, because I wanted to create a tiny alternative to the defacto Laravel Excel.

That makes sense, I think this is how many open source libraries are born here on Github. Unfortunately, many libraries also get abandoned after their first release. Mostly because developers don't want to deal with all the work that's coming in. Naturally, a library will never fit every developer's use case, and understandably, they will ask for enhancements.
Most of the time, they just expect the maintainer(s) to code whatever they want, and they feel entitled to that "service". I want to let you know that I do not think of you as someone who "has to" code the perfect library for me. This is why I started putting effort in proposing my own ideas and explaining what I believe many people could need.

It's a bit too much at the moment, and the project needs to grow by baby steps. I acknowledge that I was thinking a bit too far ahead, seems I got too excited. 😂

a (really small) competitor to the original Laravel Excel

For me, this library is actually better. Ease of use is very important to me. Furthermore, the current code base of Laravel Excel (3.0) doesn't even have an import function yet. It's scheduled for 3.1 but there is no roadmap whatsoever. It seems that the maintainers suffered a Github burnout as well, but they also have regular customer projects to take care of.

(...) did not understand it was really professionally important for you.

Yeah, although that doesn't mean you "have to" work on it with pressure. I mean yes, it would be most convenient if there was support for multiple sheets import/export already, but I can live with the wait as I already enhanced the existing code by writing my own class (as seen in this PR).

What's more important and should be the bigger motivation is the actual opportunity you seem to be missing. FastExcel, right now, has the opportunity to supersede Laravel Excel. The latter doesn't have imports right now!

OK this part is important to me. Because I opened this issue to be inclusive with new comers.

Oh, fair enough. I see that now, and it's OK.

And I failed because you thought it's not kind to do such a thing: you already have done it. So, sorry about that.

Please don't put yourself down over this - I think it is kind.
I literally (really) don't care if I already posted it in a comment, but don't worry about it 😄

My issue with it was mostly that it seemed that you waited for developers to come and help with PRs, then someone came but did something that our modern IDEs almost do automatically when creating a new file, and you applauded like this was big progress towards the goal of proper multi-sheet support. But it isn't, that's why I was confused and pointed it out.
But that it was more of a Rust Libz Blitz challenge is a great thing, and you're awesome for trying to replicate this kind of thing for newbie PHP developers.

Again, don't worry too much about what I said - most of it wasn't meant in a bad way. The word "cockblocking" was a mistake on my part, I just didn't know how to say it better. The library would already contain full multi-sheet support by now if you had said "my way" was fine. But now I don't know what you want (XYZ) and I don't want to program ABC by mistake. Hence, I waited. But nothing happened.

But I'm happy you're planning to resume work on the open discussion. Once I understand the basics behind the scenes, I will start contributing work, too, because I don't expect (nor want) you to code everything by yourself.

[rap2hpoutre]

@Harti I had a few time this morning (but no time yesterday night :/) and just commited the first part (export multiple sheets). It's available in v0.5.0. The next part will be import multi sheets.

[rap2hpoutre]

@Harti Second step done, thanks to your post yesterday :). It is now possible to import multiple sheets (the design is the one we discussed here: #24 (comment)). Let me know if something is wrong with these commits!

[rap2hpoutre]

A few more steps to come:

• Name sheets in export (see: Refactoring & Return nested Sheet Collection instead of flat Rows #24 (comment))
• Create a façade (see Refactoring & Return nested Sheet Collection instead of flat Rows #24 (comment) and Refactoring & Return nested Sheet Collection instead of flat Rows #24 (comment)).

[rap2hpoutre]

Name sheets in export fixed via #40 (commit 059442e) and available in v0.9.0.

[rap2hpoutre]

I created an issue for the last step we discussed: #52. I close this PR now, because I think everything is addressed for now (I hope I'm not wrong).

You made a real good job in order to make this lib better, thank you for this, I will remember your contribution, even if it's only visible in a closed PR!

The vast majority of the new features were made thanks to our discussion, maybe this lib is now ready for a v1.0 :)