Skip to content

Commit

Permalink
add format for nubmer with out #
Browse files Browse the repository at this point in the history
  • Loading branch information
kunashir committed Sep 1, 2015
1 parent 61881f4 commit 3d6a2d2
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 5 deletions.
2 changes: 1 addition & 1 deletion lib/spreadsheet/format.rb
View file
Expand Up @@ -105,7 +105,7 @@ def initialize opts={}
:date_or_time => Regexp.new(client("[hmsYMD]", 'UTF-8')),
:datetime => Regexp.new(client("([YMD].*[HS])|([HS].*[YMD])", 'UTF-8')),
:time => Regexp.new(client("[hms]", 'UTF-8')),
:number => Regexp.new(client("[\#]", 'UTF-8'))
:number => Regexp.new(client("([\#]|0+)", 'UTF-8'))

This comment has been minimized.

Sign in to view

Copy link
@kinkou

kinkou Dec 5, 2015

Contributor

This change made Spreadsheet return numbers from some cells that actually contain dates (and broke my code written on top of the gem). The reason behind this is that these cells have number format like [$-409]m/d/yy\ h:mm\ AM/PM;@ or [$-F800]dddd\,\ mmmm\ dd\,\ yyyy. I.e. regexp 0+ makes Spreadsheet erroneously interpret them as numbers. The values inside square brackets are locale codes. @kunashir, I appreciate your effort, but it would be naive to think that 0+ would tell numbers from other formats so easily (check out the first link below).
All in all, guys, I need your opinion on how to solve this.

This comment has been minimized.

Sign in to view

Copy link
@kunashir

kunashir Dec 5, 2015

Contributor

I was not sure in my decision, it was a quick fix. But "[#]" - is not perfect too.

This comment has been minimized.

Sign in to view

Copy link
@kinkou

kinkou Dec 5, 2015

Contributor

Ok. Since proper format detection is complicated, and implementing it would require tremendous amount of work, I would suggest the following workaround:

# lib/spreadsheet/format.rb:211

# matches '[$-409]', '[$-F800]' etc. at string beginning
NUMBER_FORMAT_LOCALE_CODE_RX = /\A\[\$\-\h{1,4}\]/

def number?
  number_format_wo_locale = @number_format.to_s.sub(NUMBER_FORMAT_LOCALE_CODE_RX, '')
  !!@regexes[:number].match(number_format_wo_locale)
end

@zdavatz, what do you think?

This comment has been minimized.

Sign in to view

Copy link
@zdavatz

zdavatz Dec 6, 2015

Owner

If you guys can work with this, ok. I would not recommend to mess around with any format detection. The user needs to handle this himself. Excel tries to be too smart in detecting formats, IMNSHO.

This comment has been minimized.

Sign in to view

Copy link
@kinkou

kinkou Dec 8, 2015

Contributor

Already done here: #155
}

# Temp code to prevent merged formats in non-merged cells.
Expand Down
5 changes: 4 additions & 1 deletion spreadsheet.gemspec
View file
@@ -1,4 +1,7 @@
require File.join(File.dirname(__FILE__), 'lib', 'spreadsheet')
# require File.join(File.dirname(__FILE__), 'lib', 'spreadsheet')
lib = File.expand_path('../lib', __FILE__)
$LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib)
require 'spreadsheet'

spec = Gem::Specification.new do |s|
s.name = "spreadsheet"
Expand Down
14 changes: 11 additions & 3 deletions test/format.rb
View file
Expand Up @@ -21,6 +21,8 @@ def test_date?
assert_equal true, @format.date?
@format.number_format = "\\$#,##0.00_);[RED]\"($\"#,##0.00\\)"
assert_equal false, @format.date?
@format.number_format = "0.00;[RED]\\-0.00"
assert_equal false, @format.date?
end
def test_date_or_time?
assert_equal false, @format.date_or_time?
Expand All @@ -31,7 +33,9 @@ def test_date_or_time?
@format.number_format = "hmsYMD"
assert_equal true, @format.date_or_time?
@format.number_format = "\\$#,##0.00_);[RED]\"($\"#,##0.00\\)"
assert_equal false, @format.date?
assert_equal false, @format.date_or_time?
@format.number_format = "0.00;[RED]\\-0.00)"
assert_equal false, @format.date_or_time?
end
def test_datetime?
assert_equal false, @format.datetime?
Expand All @@ -44,7 +48,9 @@ def test_datetime?
@format.number_format = "HSYMD"
assert_equal true, @format.datetime?
@format.number_format = "\\$#,##0.00_);[RED]\"($\"#,##0.00\\)"
assert_equal false, @format.date?
assert_equal false, @format.datetime?
@format.number_format = "0.00;[RED]\\-0.00)"
assert_equal false, @format.datetime?
end
def test_time?
assert_equal false, @format.time?
Expand All @@ -59,7 +65,9 @@ def test_time?
@format.number_format = "hms"
assert_equal true, @format.time?
@format.number_format = "\\$#,##0.00_);[RED]\"($\"#,##0.00\\)"
assert_equal false, @format.date?
assert_equal false, @format.time?
@format.number_format = "0.00;[RED]\\-0.00)"
assert_equal false, @format.time?
end
def test_borders?
assert_equal [:none, :none, :none, :none], @format.border
Expand Down

1 comment on commit 3d6a2d2

@zdavatz
Copy link
Owner

@zdavatz zdavatz commented on 3d6a2d2 Dec 6, 2015

Choose a reason for hiding this comment

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

send me a pull request please and make sure the tests pass.

Please sign in to comment.