Fix worksheet title length enforcement #70
Conversation
Opposes a redundant test
| @@ -663,7 +663,8 @@ def outline(collection, range, level = 1, collapsed = true) | |||
|
|
|||
| def validate_sheet_name(name) | |||
| DataTypeValidator.validate :worksheet_name, String, name | |||
| raise ArgumentError, (ERR_SHEET_NAME_TOO_LONG % name) if name.bytesize > 31 | |||
| character_length = name.encode("utf-16").bytesize / 2 - 1 | |||
There was a problem hiding this comment.
I do not completely understand why the - 1 is necessary 🤔
There was a problem hiding this comment.
Maybe we can skip the division and simply use … < 32?
There was a problem hiding this comment.
Tried it out. That would make the tests green, but then the code would look like this
character_length = name.encode("utf-16").bytesize / 2
raise ArgumentError, (ERR_SHEET_NAME_TOO_LONG % name) if character_length > 32the code walk talk as if 32 is the character limit which it is not :/
There was a problem hiding this comment.
It seems that Ruby adds a BOM at the beginning of a string when encoding it as UTF-16:
"123".encode("utf-16") # => "\uFEFF123"The BOM counts for 2 bytes, but is probably irrelevant to Excel’s length restriction. If that’s the case, then something like this should work:
utf16_name = name.encode("utf-16")[1..-1] # ignore first character (BOM) because Excel does so, too.
raise … if utf16_name.bytesize > 64 # or `>= 64`? I’m confused … There was a problem hiding this comment.
thanks for that investigative work 🕵️ I still use the / 2 because the 31 is pretty valuable to have there written as a number
by the way, using Axlsx::coder.encode (from below) also does not help :D
| @@ -663,7 +663,8 @@ def outline(collection, range, level = 1, collapsed = true) | |||
|
|
|||
| def validate_sheet_name(name) | |||
| DataTypeValidator.validate :worksheet_name, String, name | |||
| raise ArgumentError, (ERR_SHEET_NAME_TOO_LONG % name) if name.bytesize > 31 | |||
| character_length = name.encode("utf-16").bytesize / 2 - 1 | |||
There was a problem hiding this comment.
Maybe we can skip the division and simply use … < 32?
- The previous change caused unnecessary issues - We approximate that Excel calculates the character length with UTF-16 - Fixes #67
|
@noniq Are we happy with the changes on this pull request? |
|
The expression could remove division and retain some clarity like this: But I don't really see any gains by improving these lines. Certainly performing division by 2 for each sheet name is not a significant performance hit. Since other things are waiting on this and tests are passing, I'm going to merge this request. We can revisit this syntax in another pull request. |
straydogstudio
dismissed
noniq’s stale review
I'm setting this aside for now since tests are passing. We can revisit these lines and improve them in another pull request. Sorry if I'm out of line @noniq.
|
Released in 3.0.4 |
Closes #67