Use worksheet without block

[sandstrom]

Successor to #42

Credit to @kukicola

[felixbuenemann]

Could you describe how the commit "Track worksheet with WorksheetEntry" contributes to the close checking?

The commit message says it is needed, but I can't really see a relation the the close checking code.

Also why did you change the author on the initial commit by @kukicola?

[sandstrom]

Thanks @felixbuenemann

It's needed because we need a reference to the worksheet, to check the closed? method, since only the worksheet knows whether it has been closed or not.

I never changed it, I just copied the diff. Couldn't work out how to fork his fork so I ended up recreating the commit instead. I wrote "credit @kukicola" in the commit message though. If it's important I can google and see if I can change the author via git CLI.

[sandstrom]

@felixbuenemann I've managed to change the author of the commit (I think)

I've also made the change backwards-compatible.

Let me know what you think!

[kukicola]

@felixbuenemann @sandstrom Hi guys! I like the idea of having those errors, good job! One thing that bothers me here: You are using WorksheetEntry to store a name and reference to a worksheet. Before it was a hash with id as a key and name as a value. Maybe instead of adding WorksheetEntry you can just use hash with id as a key and reference as a value, and store the name of the worksheet inside Worksheet class? I think it would be simpler. What do you think?

[sandstrom]

@kukicola Thanks! And thanks for bringing this up in the first place!

I actually wrote it that way first, but that would require more changes to the code since worksheets are created "on their own" in the tests. So to keep the change delta down I did this instead.

Don't have a strong preference though, if felix would like it that way instead I'm happy to change.

[felixbuenemann]

I think the most straighjt forward solution would be to just add the id and name properties to the Worksheet class and then just use an array of worksheet instances.

Since we store a referece to the worksheet class anyways, I don't see a reason to wrap it in a struct.

Am I missing something?

[sandstrom]

@felixbuenemann Alright, I'll go with that.

We'd need to pass in "dummy" id and name in worksheet_test.rb, but it's fairly straightforward.

[sandstrom]

@felixbuenemann @kukicola I've updated the PR, let me know what you think!

[felixbuenemann]

We'd need to pass in "dummy" id and name in worksheet_test.rb, but it's fairly straightforward.

I don't think we really need that, if we use default values.

I also don't quite like that we have the default value for name handled outside the worksheet class, when we could just pass it through via the options hash.

But I can take care of those when merging the code.

[sandstrom]

@felixbuenemann Thanks Felix!

Do/change whatever you like here. You have commit rights to this branch (in my fork) if that helps.

Thanks for a great library! 🏅

[felixbuenemann]

@sandstrom @kukicola Thanks for your work on this, I've merged it with a few tweaks and fixes.

[felixbuenemann]

This feature is now released in xlsxtream 2.4.0.

[sandstrom]

Awesome, thanks @felixbuenemann 🎉