Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Roo - Moving Forward #242

Closed
stevendaniels opened this issue Jul 20, 2015 · 13 comments
Closed

Roo - Moving Forward #242

stevendaniels opened this issue Jul 20, 2015 · 13 comments

Comments

@stevendaniels
Copy link
Contributor

I've spent a lot of time thinking about Roo and how Roo can be improved. Here's a summary of the ideas I'd like to see implemented going forward.

Backwards Compatibility

I found Roo the way many of you probably found it: Ryan Bates' RailsCasts. I've always been hesitant to introduce major changes to Roo because it would make Roo incompatible with all of the screencasts and tutorials that teach developers to use Roo. That's why I think major upgrades should allow some compatibility mode for developers who prefer Roo's classic API. I envision something like this:

require 'roo'
Roo.use_roo_classic!

Or

require 'roo'
require 'roo/classic'

Common Interface across supported file types.

Format interfaces aren't the same. CSV works very differently from Excelx, which is very different from OpenOffice. The interfaces for roo-xls and roo-google are also different.

This made sense when these formats were being added to Roo, but now, it makes adding new features difficult because they usually need to be implemented in several different places.

More conventional API.

Roo's API doesn't always work the way I'd expect it to. (Issue #223 is an example of this problem.)

I also think Roo::Excelx does far too much. It interacts with Sheets and Cells and it's API is all over the place. I feel that many of Roo's public methods (like first_row, last_row, etc.) are used to do things that an enumerator could do.

That's why I'd like to see sheets, rows, columns, and cells work like enumerators

workbook = Roo.open file 
# <Roo::Workbook @filename=file>
workbook.sheets.first 
# =>  <Roo::WorkSheet @name="Sheet 1">

sheet = workbook['Sheet 1']
# => <Roo::WorkSheet @name="Sheet 1">    
row = sheet.rows.first # returns cells from the first row
row.cells.last # returns the last cell from the row.

Cells would work similar to pull request #240.

cell = sheet['a24']    # returns cell at A24
cell.value   #  <Date: 2015-01-25>
cell.cell_value  # 42029
cell.formatted_value   # '01-25-15'

Roo 2.x.x

I'd like to add these changes into Roo in the 2.x.x version. This would allow developers to continue to use Roo as is, but also allow Roo 2.x.x developers to do something like Roo.use_new_roo! to take advantage of the new API (which would be similar to the one I briefly outlined above).

Once we got to Roo 3.0.0, Roo's default behavior would be the new API, and developers would need to use Roo.use_roo_classic! to use Roo's classic API.


I'd love to hear everyone's thoughts.

@sergiopantoja
Copy link

Common Interface across supported file types.
I'd like to see sheets, rows, columns, and cells would work like enumerators

Yes and yes! 👍 I love both of these ideas. Roo.use_new_roo! seems fine to me for 2.x.x.

@kernelsmith
Copy link

@stevendaniels It's funny you bring this stuff up, I was just about to either open issue or a PR for one particular instance of the "more like enumerations" issue. My particular problem is the sheets method does not return the actual sheets, but rather just the sheet names. AFAICT, there's no way (w/o accessing private vars/methods) to get a reference to a spreadsheet's underlying worksheet (I admit I'm not overly familiar w/the codebase however), e.g.:

ss = Roo::Excelx.new('some.xlsx')
ws = ss.sheet('Worksheet Name')
ws.class  # => Roo::Excelx

I was originally thinking of PR'ing a change whereby the sheets method returned the sheet objects in an array, in stead of the names, however I figure that will break someone's codes (tho it seems far more natural to me). Instead, providing an each_sheet method that expects a block would give access to the sheet objects w/o breaking the current API). Let me know what you think and if you'd like that PR, I will submit it

@stevendaniels
Copy link
Contributor Author

I definitely think the functionality is needed, but because it isn't consistent with Roo's classic API, it doesn't make sense to add it.

I've been working on "new Roo" API (nothing online yet), and it already works with xlsx files, but I need to add compatibility with Roo's other formats and then restructure my changes into smaller commits (rather than one huge multi-file commit).

If you need to enumerate over sheets, this hack will work:

class Roo::Excelx
  def each_sheet
    sheets.map do |name|
      sheet_for(name)
    end
  end
end

@simonoff
Copy link
Member

simonoff commented Aug 1, 2015

I don't like idea use_new_roo!. It will not inform old users what API will change.The better approach is a stubs for old interface with message what it's deprecated.

@stevendaniels
Copy link
Contributor Author

Because of the size of the API, I was thinking of creating new namespaces for new Roo, like Roo::XLSX and Roo::ODS (Roo::CSV is more problematic ). 

So, for example, the entire Roo::Excelx namespace would be deprecated. In 3.0.0, `Roo.use_roo_classic! would stub the old interface and namespaces.

@simonoff
Copy link
Member

simonoff commented Aug 1, 2015

No, in 2.x you need to deprecate all old calls and says what it will be changed. And only in 3.x you can remove old calls

@stevendaniels
Copy link
Contributor Author

Here's what I'm thinking:

2.x

  • add the new API using new namespaces. The new API is created using shared modules that are extracted from the old API.
  • The old API would still be the primary API.
  • Give Roo.open a deprecation warning.

3.x

  • Roo.open returns the new namespaces (e.g. Roo::XLSX, etc.) instead of the classic api.
  • classic API is still accessible using a method like Roo.use_roo_classic! or using a config block.
  • methods for the classic API are stubbed using the new API's methods.
  • The classic API is given deprecation warnings.

4.x

  • classic API is removed from Roo.
    (I currently have no feature plans for 4.x)

I don't want to remove the classic API in 3.x because I it would break too many projects.

@sergiopantoja
Copy link

@simonoff has a good point. It boils down to whether we follow semantic versioning or not. If we do, breaking API changes would be acceptable and actually expected with a 3.0.0 release. From the SemVer spec:

Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API. ...

Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. It MUST be incremented if any public API functionality is marked as deprecated. ...

How should I handle deprecating functionality?

Deprecating existing functionality is a normal part of software development and is often required to make forward progress. When you deprecate part of your public API, you should do two things: (1) update your documentation to let users know about the change, (2) issue a new minor release with the deprecation in place. Before you completely remove the functionality in a new major release there should be at least one minor release that contains the deprecation so that users can smoothly transition to the new API.

I would prefer that we follow SemVer.

@simonoff
Copy link
Member

simonoff commented Aug 1, 2015

@stevendaniels 3.x in any case will have different API. About braking a lot of projects - All knowns about SemVer so it's their own issue. I'm on all my rails projects using gem 'roo', '~> 2.0' so i will be sure what it will not install 3.x and I will not have incompatibilities.

@stevendaniels
Copy link
Contributor Author

I'm not suggesting that we abandon SemVer. And @simanoff is right, those who use SemVer in their Gemfile won't have any issues. But it will break some projects and it will also break all of the tutorials and screencasts.

Developers who come to Roo from those resources will find a project completely incompatible with the stuff they just learned. I want to put off alienating those developers as long as possible.

SemVer:

Before you completely remove the functionality in a new major release there should be at least one minor release that contains the deprecation so that users can smoothly transition to the new API.

In my reading, SemVer doesn't define how long it should take before removing deprecated functionality: it should take at least one minor release, but it could also take one major release, too. That's my reasoning behind waiting until 4.x to remove the classic Roo API.

This isn't a sticking point for me. If you have a strong preference for removing the classic API completely for a 3.x release, then we can do that.

@simonoff
Copy link
Member

simonoff commented Aug 2, 2015

Wait. If you will look screencast for rails 2.x and then you will try to use such screenscast as tutorial you will get mess.

@jarredholman
Copy link

Has any work on this happened yet?

@stevendaniels
Copy link
Contributor Author

Started to make some progress down this long road.

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

No branches or pull requests

5 participants