Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(227)

Issue 238520043: Remove CR LF from snippets using makelsr (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 10 months ago by PhilEHolmes
Modified:
8 years, 8 months ago
Reviewers:
dak, mail
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Some of the snippets imported from the LSR (notably some of the headwords) have CR LF at the end of their lines. This appears to be viewed as excess whitespace and stripped when the git repo is updated. As a result, any import of the LSR flags these snippets as changed. This simple fix replaces CR LF with LF during the import with makelsr. This fixes the excess whitespace problem.

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -0 lines) Patch
M scripts/auxiliar/makelsr.py View 1 chunk +1 line, -0 lines 1 comment Download

Messages

Total messages: 3
PhilEHolmes
Please review.
8 years, 10 months ago (2015-05-24 13:56:33 UTC) #1
dak
https://codereview.appspot.com/238520043/diff/1/scripts/auxiliar/makelsr.py File scripts/auxiliar/makelsr.py (right): https://codereview.appspot.com/238520043/diff/1/scripts/auxiliar/makelsr.py#newcode254 scripts/auxiliar/makelsr.py:254: s = s.replace ("\r\n", "\n") It would seem that ...
8 years, 10 months ago (2015-05-24 14:52:53 UTC) #2
mail_philholmes.net
8 years, 10 months ago (2015-05-24 15:05:45 UTC) #3
----- Original Message ----- 
From: <dak@gnu.org>
To: <PhilEHolmes@googlemail.com>
Cc: <reply@codereview-hr.appspotmail.com>; <lilypond-devel@gnu.org>
Sent: Sunday, May 24, 2015 3:52 PM
Subject: Re: Remove CR LF from snippets using makelsr (issue 238520043 
byPhilEHolmes@googlemail.com)


>
> https://codereview.appspot.com/238520043/diff/1/scripts/auxiliar/makelsr.py
> File scripts/auxiliar/makelsr.py (right):
>
>
https://codereview.appspot.com/238520043/diff/1/scripts/auxiliar/makelsr.py#n...
> scripts/auxiliar/makelsr.py:254: s = s.replace ("\r\n", "\n")
> It would seem that this replacement should be done before other
> line-related replacements.  Basically, right after reading s.  At least
> I'm not sure that it does not affect the other replacements.
>
> Most other replacements appear to be precompiled (look for
> strip_white_spaces_re for an example).  It may make sense doing that
> here as well.
>
> https://codereview.appspot.com/238520043/

I took the view that, since the snippets are left with these artefacts 
_after_ all the other replacements have been completed, it would be best to 
remove them after all the other ones, too.  I still believe that to be true.

I also considered that the other replacements required regular expressions, 
but this was so simple there was no point.  After all, you can't have CR LF 
except at the end of the line.  You can have whitespace in lots of other 
places.

FWIW I have tried it and it does fix the problem with the current LSR import 
and does not mess anything else up.

--
Phil Holmes 

Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b