search

roo-rb / roo

Roo provides an interface to spreadsheets of several sorts.
MIT License
2.79k stars 501 forks source link

Removes link attr_reader from Cells #457

Closed tgturner closed 6 years ago

tgturner commented 6 years ago

In https://github.com/roo-rb/roo/pull/449 we remove the link instance variable (as we should have), but we left the reader's in which would always return nil. Added in deprecation warning to point people in the correct direction.

tgturner commented 6 years ago

As a side note, the Link class doesn't appear to be exceptionally useful. Opinions on removing it? My vote would be to remove support for having hyperlinks in an excel file at all. I can't see an instance where knowing the link would be very useful (except for possibly re-exporting the file?).

What are your thoughts on this?

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.2%) to 93.781% when pulling d43cce0fef8ee62459501e10606545aad229c5b4 on tgturner:remove-link-reader into 0a2e800174859e313846d0cb83c765644f99e7cc on roo-rb:master.

chopraanmol1 commented 6 years ago

@tgturner I don't think Link class is useless. It does add a few methods to the cell's value object url. href, to_uri which other people may be relying upon.

If your concern is regarding performance then the right course of action will be to extend the no_hyperlinks option to the non-streaming method as well.

no_hyperlinks option's default value is false we can make it true during next major release (3.0.0).

I also think most people do not use hyperlinks, but removing the functionality altogether is something I believe we shouldn't do.

tgturner commented 6 years ago

@chopraanmol1 Made the suggested fixes.

Regarding hyperlinks, I think that not reading hyperlinks by default is probably a great option