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.

Sign up for GitHub

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

Jump to bottom

Removes link attr_reader from Cells #457

Merged
merged 3 commits into from
Sep 19, 2018
Merged

Removes link attr_reader from Cells #457

merged 3 commits into from
Sep 19, 2018

Conversation

tgturner
Copy link
Contributor

@tgturner tgturner commented Sep 18, 2018 •
edited
Loading

In #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
Copy link
Contributor Author

tgturner commented Sep 18, 2018 •
edited
Loading

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
Copy link

coveralls commented Sep 18, 2018 •
edited
Loading

Coverage Status

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

chopraanmol1
chopraanmol1 reviewed Sep 19, 2018
View reviewed changes
Copy link
Member

@chopraanmol1 chopraanmol1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

lib/roo/excelx/cell/base.rb

# DEPRECATED: Please use link? instead.
def link
warn '[DEPRECATION] `link` is deprecated. Please use `link?` instead.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also return expected value with a deprecation warning.
Which should be link?

But I think storing a boolean value to @link was a bug.

if we consider previous implementation of link buggy then link method must be as follow:

def link
  link? ? value : nil
end

or it should be as follow:

def link
  warn '[DEPRECATION] `link` is deprecated.  Please use `link?` instead.'
  link?
end

lib/roo/excelx/cell/base.rb
def hyperlink
warn '[DEPRECATION] `hyperlink` is deprecated. Please use `link` instead.'
warn '[DEPRECATION] `hyperlink` is deprecated. Please use `link?` instead.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should return link or link?

@chopraanmol1
Copy link
Member

@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
Copy link
Contributor Author

@chopraanmol1 Made the suggested fixes.

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

chopraanmol1
chopraanmol1 approved these changes Sep 19, 2018
View reviewed changes
Copy link
Member

@chopraanmol1 chopraanmol1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chopraanmol1 chopraanmol1 merged commit 938bbf4 into roo-rb:master Sep 19, 2018
@tgturner tgturner deleted the remove-link-reader branch September 21, 2018 16:23
chopraanmol1 added a commit to chopraanmol1/roo that referenced this pull request Sep 25, 2018
@chopraanmol1
Extend no_hyperlinks option to non-streaming methods of Excelx
dcb97aa 
Discussed in roo-rb#457 (comment)

No noticeable performance or memory benefits.
I think roo-rb#456 must have solved most performance and memory issue regarding extract_hyperlinks.
@chopraanmol1 chopraanmol1 mentioned this pull request Sep 25, 2018
Merged
tgturner pushed a commit that referenced this pull request Sep 27, 2018
@chopraanmol1 @tgturner
Extend no_hyperlinks option to non-streaming methods of Excelx (#459)
ecfb74f 
Discussed in #457 (comment)

No noticeable performance or memory benefits.
I think #456 must have solved most performance and memory issue regarding extract_hyperlinks.
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jan 20, 2019
math/ruby-roo: update to 2.8.0
4b418c8 
pkgsrc change: add "USE_LANGUAGES= # none".

##  [2.8.0] 2019-01-18
### Fixed
- Fixed inconsistent column length for CSV [375](roo-rb/roo#375)
- Fixed formatted_value with `%` for Excelx [416](roo-rb/roo#416)
- Improved Memory consumption and performance [434](roo-rb/roo#434) [449](roo-rb/roo#449) [454](roo-rb/roo#454) [456](roo-rb/roo#456) [458](roo-rb/roo#458) [462](roo-rb/roo#462) [466](roo-rb/roo#466)
- Accept both Transitional and Strict Type for Excelx's worksheets [441](roo-rb/roo#441)
- Fixed ruby warnings [442](roo-rb/roo#442) [476](roo-rb/roo#476)
- Restore support for URL as file identifier for CSV [462](roo-rb/roo#462)
- Fixed missing location for Excelx's links [482](roo-rb/roo#482)

### Changed / Added
- Drop support for ruby 2.2.x and lower
- Updated rubyzip version for fixing security issue. Now minimal version is 1.2.1
- Roo::Excelx::Coordinate now inherits Array [458](roo-rb/roo#458)
- Improved Roo::HeaderRowNotFoundError exception's message [461](roo-rb/roo#461)
- Added `empty_cell` option which by default disable allocation for Roo::Excelx::Cell::Empty [464](roo-rb/roo#464)
- Added support for variable number of decimals for Excelx's formatted_value [387](roo-rb/roo#387)
- Added `disable_html_injection` option to disable html injection for shared string in `Roo::Excelx` [392](roo-rb/roo#392)
- Added image extraction for Excelx [414](roo-rb/roo#414) [397](roo-rb/roo#397)
- Added support for `1e6` as scientific notation for Excelx [433](roo-rb/roo#433)
- Added support for Integer as 0 based index for Excelx's `sheet_for` [455](roo-rb/roo#455)
- Extended `no_hyperlinks` option for non streaming Excelx methods [459](roo-rb/roo#459)
- Added `empty_cell` option to disable Roo::Excelx::Cell::Empty allocation for Excelx [464](roo-rb/roo#464)
- Added support for Integer with leading zero for Roo:Excelx [479](roo-rb/roo#479)
- Refactored Excelx code [453](roo-rb/roo#453) [477](roo-rb/roo#477) [483](roo-rb/roo#483) [484](roo-rb/roo#484)

### Deprecations
- Roo::Excelx::Sheet#present_cells is deprecated [454](roo-rb/roo#454)
- Roo::Utils.split_coordinate is deprecated [458](roo-rb/roo#458)
- Roo::Excelx::Cell::Base#link is deprecated [457](roo-rb/roo#457)
aravindm pushed a commit to chobiwa/roo that referenced this pull request Jun 18, 2019
@tgturner @aravindm
Removes link attr_reader from Cells (roo-rb#457)
49af97c 
* Removes link attr_reader from Cells

* Adds deprecation method for link

* Deprecated methods should still return a value
aravindm pushed a commit to chobiwa/roo that referenced this pull request Jun 18, 2019
@chopraanmol1 @aravindm
Extend no_hyperlinks option to non-streaming methods of Excelx (roo-r…
79729c1 
…b#459)

Discussed in roo-rb#457 (comment)

No noticeable performance or memory benefits.
I think roo-rb#456 must have solved most performance and memory issue regarding extract_hyperlinks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chopraanmol1 chopraanmol1 chopraanmol1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tgturner @coveralls @chopraanmol1