convert-ly (issue 2670)

convert-ly:

- Exit with error status when errors occur.

- Use unicode strings for file names printing.

- Don't update \version when no rule is applied.

This fixes issue 2670.

[Julien Rioux, 2012-10-06 01:56:26]

For review:

[lemzwerg, 2012-10-06 05:06:56]

http://codereview.appspot.com/6610058/diff/2001/scripts/convert-ly.py
File scripts/convert-ly.py (right):

http://codereview.appspot.com/6610058/diff/2001/scripts/convert-ly.py#newcode357
scripts/convert-ly.py:357: sys.exit(errors)
Are you going to report the number of errors using the `errors' variable?  In
case this is true, I would consider this a bad idea, since you abuse the
functionality of the exit status.

How large can `errors' become?  The value returned by `exit' must be in the
range 0-255 (with 0 meaning `no error').

[Julien Rioux, 2012-10-06 07:07:35]

On Sat, Oct 6, 2012 at 1:06 AM, <lemzwerg@googlemail.com> wrote:

> Are you going to report the number of errors using the `errors'
> variable?  In case this is true, I would consider this a bad idea, since
> you abuse the functionality of the exit status.
>
> How large can `errors' become?  The value returned by `exit' must be in
> the range 0-255 (with 0 meaning `no error').
>
>
http://codereview.appspot.com/**6610058/<http://codereview.appspot.com/6610058/>
>

I thought it would be interesting to track the number of errors, yes, but
you are absolutely right. I`ll change it to return 0 (success) or 1
(failure).

Cheers,
Julien

[lemzwerg, 2012-10-06 18:02:48]

LGTM now.

[janek, 2012-10-08 04:58:06]

http://codereview.appspot.com/6610058/diff/9001/scripts/convert-ly.py
File scripts/convert-ly.py (right):

http://codereview.appspot.com/6610058/diff/9001/scripts/convert-ly.py#newcode231
scripts/convert-ly.py:231: ly.progress (_ (u"Processing `%s\'... ") %
infile_name, True)
is 'u' (here and in some other places) intended?  If so, what does it do? (a
comment maybe?)

[dak, 2012-10-08 05:05:06]

On 2012/10/08 04:58:06, janek wrote:
> http://codereview.appspot.com/6610058/diff/9001/scripts/convert-ly.py
> File scripts/convert-ly.py (right):
> 
>
http://codereview.appspot.com/6610058/diff/9001/scripts/convert-ly.py#newcode231
> scripts/convert-ly.py:231: ly.progress (_ (u"Processing `%s\'... ") %
> infile_name, True)
> is 'u' (here and in some other places) intended?  If so, what does it do? (a
> comment maybe?)

I don't think we should be documenting the Python language, others do a better
job at that.  Check out
<URL:http://docs.python.org/reference/lexical_analysis.html#strings>

[janek, 2012-10-08 05:15:04]

On Mon, Oct 8, 2012 at 7:05 AM,  <dak@gnu.org> wrote:
> I don't think we should be documenting the Python language, others do a
> better job at that.  Check out
> <URL:http://docs.python.org/reference/lexical_analysis.html#strings>

ah, ok. thanks!

[Graham Percival, 2012-10-08 20:08:20]

The changelog says
"Don't update \version when no rule is applied."

That's what the existing -d --diff-version-update command does.  If this is
intended to be the default behaviour now, then the command-line option should be
removed.  I would rather keep convert-ly as-is (in terms of this behaviour; the
other changes in this patch look good) and let users chose whether they want -d
or not.  However, if there's strong feeling that -d should be the default, then
could we replace that command-line option and give it a separate -u
--always-update-version or something like that?

[Julien Rioux, 2012-10-08 20:37:36]

On Mon, Oct 8, 2012 at 4:08 PM,  <graham@percival-music.ca> wrote:
> The changelog says
>
> "Don't update \version when no rule is applied."
>
> That's what the existing -d --diff-version-update command does.  If this
> is intended to be the default behaviour now, then the command-line
> option should be removed.

No, -d does not become the default; the behavior of convert-ly is
unchanged when rules are being applied, and -d is still used in the
same way. The change concerns itself only with the case that no rule
applies, because the version of the file is already up-to-date. In
this case, it used to be that the version in the file would be set to
version of the last rule, which would sometimes mean that the version
of the file upon output is lower than the version at input. This is
what we avoid in this patch.

Please consult http://code.google.com/p/lilypond/issues/detail?id=2670
and references therein.

Cheers,
Julien

[dak, 2012-10-08 20:38:36]

On 2012/10/08 20:08:20, Graham Percival wrote:
> The changelog says
> "Don't update \version when no rule is applied."
> 
> That's what the existing -d --diff-version-update command does.

No, that's what the existing -d --diff-version-update is supposed to do.

The problem was that if the last applicable rule was for version 2.16.6 and you
asked for an upgrade to version 2.16.9 and the file claimed to be 2.16.9
already, its version was set to 2.16.6 instead of staying at 2.16.9.  That's
what the issue was about in the first place.  I assume that this is what has
been fixed.