groovy

DGM#denormalize - Mac style line breaks and empty string

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.6-rc-1
  • Fix Version/s: 1.6-rc-2, 1.5.8, 1.7-beta-1
  • Component/s: groovy-jdk
  • Labels:
    None
  • Environment:
    Groovy 1.5.8, 1.6, trunk.
  • Number of attachments :
    2

Description

DGM#denormalize, in contrast to DGM#normalize, does not handle Mac style line breaks correctly. It throws an IndexOutOfBoundsException with empty strings.

Attached a patch that solves this. It does not need an import of PlatformLineWriter and throws no IOException anymore.

The patch also contains an alternative implementation of DGM#normalize it has speedup factors of 0.66 to 8 compared with the current implementation. However I don't know if it would have any practical worth to replace the current implementation.

The second patch adds tests to DefaultGroovyMethodsTest.

Additionally, I would like to discuss if DGM#normalize should really remove the last trailing linefeed.

  1. Text File
    patch-DGMTest-trunk.txt
    07/Jan/09 8:49 AM
    3 kB
    Alexander Veit
  2. Text File
    patch-DGM-trunk.txt
    07/Jan/09 8:49 AM
    3 kB
    Alexander Veit

Issue Links

relates to

New Feature - A new feature of the product, which has yet to be developed. GROOVY-3246 There should be a DGM.normalize(String) method which acts like readLines().join('\n')

  • Minor - Minor loss of function, or other problem where easy workaround is present.
  • Closed - The issue is considered finished, the resolution is correct. Issues which are not closed can be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Alexander Veit added a comment -

Other than the names of the patch files pretend they're not for trunk but for the 1.6 branch.

Show
Alexander Veit added a comment - Other than the names of the patch files pretend they're not for trunk but for the 1.6 branch.
Hide
Permalink
Jim White added a comment -

I would like denormalize to be defined strictly in terms of replacing

{'\n'}

with the platform line separator.

See GROOVY-3246 for the proposed patch.

Show
Jim White added a comment - I would like denormalize to be defined strictly in terms of replacing {'\n'} with the platform line separator. See GROOVY-3246 for the proposed patch.
Jim White made changes -
Field Original Value New Value
Link This issue relates to GROOVY-3246 [ GROOVY-3246 ]
Hide
Permalink
Jim White added a comment -

Ah, I've actually read this issue now, and I agree that normalize should not strip the trailing line separator. The patch tidies all this stuff up.

Show
Jim White added a comment - Ah, I've actually read this issue now, and I agree that normalize should not strip the trailing line separator. The patch tidies all this stuff up.
Hide
Permalink
Jim White added a comment -

I don't think denormalize should replace CR with line separators. The reason is that a normalized string will only have LF for line termination. If you've got a string that isn't normalized (i.e. may have CR for line termination), then you should use normalize on it first.

Show
Jim White added a comment - I don't think denormalize should replace CR with line separators. The reason is that a normalized string will only have LF for line termination. If you've got a string that isn't normalized (i.e. may have CR for line termination), then you should use normalize on it first.
Hide
Permalink
Alexander Veit added a comment -

Your current implementation of denormalize, as well as the attached patch, replaces single CR with line separators.

The old implemetation only replaced LF and CR LF line separators.

So the current implementation seems to be more logical to me.

Show
Alexander Veit added a comment - Your current implementation of denormalize , as well as the attached patch, replaces single CR with line separators. The old implemetation only replaced LF and CR LF line separators. So the current implementation seems to be more logical to me.
Hide
Permalink
Paul King added a comment -

OK, obviously we need to fix the IOOBE and handle Mac style line breaks. My preferred goal would be that if I used getText() followed by normalize() followed by denormalize() then I would leave the original line endings "mostly intact".

Show
Paul King added a comment - OK, obviously we need to fix the IOOBE and handle Mac style line breaks. My preferred goal would be that if I used getText() followed by normalize() followed by denormalize() then I would leave the original line endings "mostly intact".
Hide
Permalink
Jim White added a comment - - edited

I changed my mind and decided that handling all line separators was the right thing to do.

It also turned out to be very easy because it just involved putting in a call to normalize (which I was trying to claim the user ought to do - my bad) and that is fairly quick if there is little overhead if there are no CR in the string.

And getText().normalize().denormalize() will return exactly the original string if it used the platform line separators exclusively. The last line will have a line separator or not just as the original string did. Of course getText().normalize().denormalize() (and getText().normalize().normalize().denormalize()) is the same as getText().denormalize() because denormalize does a normalize rather than assuming LF for line separation. See the test cases in groovy.StringTest and groovy.bugs.Groovy3235Bug.

Show
Jim White added a comment - - edited I changed my mind and decided that handling all line separators was the right thing to do. It also turned out to be very easy because it just involved putting in a call to normalize (which I was trying to claim the user ought to do - my bad) and that is fairly quick if there is little overhead if there are no CR in the string. And getText().normalize().denormalize() will return exactly the original string if it used the platform line separators exclusively. The last line will have a line separator or not just as the original string did. Of course getText().normalize().denormalize() (and getText().normalize().normalize().denormalize() ) is the same as getText().denormalize() because denormalize does a normalize rather than assuming LF for line separation. See the test cases in groovy.StringTest and groovy.bugs.Groovy3235Bug .
Jim White made changes -
Status Open [ 1 ] Resolved [ 5 ]
Resolution Fixed [ 1 ]
Fix Version/s 1.5.8 [ 14630 ]
Fix Version/s 1.7-beta-1 [ 14014 ]
Fix Version/s 1.6 [ 13832 ]
Paul King made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

  • Assignee:
    Unassigned
    Reporter:
    Alexander Veit
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: